-
Notifications
You must be signed in to change notification settings - Fork 84
Libarchive and DRN Improvements #862
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
- Storm can now be linked with libarchive as an optional dependency - Auxiliary functions to conveniently read and write binary and textual files from and to an archive - allow to control the precision of floats in DRN export using --io:digits option - allow reading from and writing to compressed DRN files (use .drn.gz / drn.xz file extensions and/or the new --compression option) (requires libarchive)
…arsed values. - support for parsing of DRN interval models from CLI (if they have a @value_type section)
|
|
| # following https://stackoverflow.com/a/76916842 | ||
| # Homebrew ships libarchive keg only, include dirs have to be set manually | ||
| execute_process( | ||
| COMMAND brew --prefix libarchive |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumes that it has been installed with brew? Can/Should we maybe first check whether it is somehow already present in the system otherwise via find_package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I now see that it is always searched fore and that we are only setting a prefix variable, which is implicitly used by the findpackage below? I am not sure using PREFIX is better than passing this as a hint to the find package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seconded. There are people on mac not using homebrew ;).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the execute_process is used to get a hint to catch the common case where brew is used. It fails quietly if brew is not installed.
I added some comments to clarify this.
Any advice on how to avoid this brew hack? And/or adding a hint to the find_package?
Setting LibArchive_INCLUDE_DIR seems to work most of the time, but I'd rather set the hint.
Relevant:
> brew info libarchive
[...]
libarchive is keg-only, which means it was not symlinked into /opt/homebrew,
because macOS already provides this software and installing another version in
parallel can cause all kinds of trouble.
If you need to have libarchive first in your PATH, run:
echo 'export PATH="/opt/homebrew/opt/libarchive/bin:$PATH"' >> ~/.zshrc
For compilers to find libarchive you may need to set:
export LDFLAGS="-L/opt/homebrew/opt/libarchive/lib"
export CPPFLAGS="-I/opt/homebrew/opt/libarchive/include"
For pkgconf to find libarchive you may need to set:
export PKG_CONFIG_PATH="/opt/homebrew/opt/libarchive/lib/pkgconfig"
[...]The version shipped with macOS apparently does not have any include files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use the HINTS for find_package instead of setting the include directory? Alternatively, you could try setting LibArchive_ROOT.
Last idea could be to write a FindLibArchive.cmake which contains the homebrew workaround. This might then also work if we install Storm.
resources/3rdparty/CMakeLists.txt
Outdated
| set(LibArchive_INCLUDE_DIR "${LIBARCHIVE_PREFIX}/include") | ||
| else() | ||
| message(STATUS "Storm - LibArchive not found in ${LIBARCHIVE_PREFIX}/include.") | ||
| endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have an else case here that also reports that libarchive is not found?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That output was just to see if the brew hint was successful. I removed it because it is confusing.
There is always some kind of output after find_package
resources/3rdparty/CMakeLists.txt
Outdated
| endif() | ||
| else() | ||
| SET(STORM_HAVE_LIBARCHIVE OFF) | ||
| message (WARNING "Storm - Not linking with LibArchive. Loading and exporting compressed files such will not be supported.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a status or a warning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
STATUS seems to be more in line with other dependencies.
I think it should be |
| else() | ||
| SET(STORM_HAVE_LIBARCHIVE OFF) | ||
| message (WARNING "Storm - Not linking with LibArchive. Loading and exporting compressed files such will not be supported.") | ||
| endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warning message above may explicitly mention UMB (even uncompressed, as it is packed!)
| ) | ||
| endif() | ||
|
|
||
| if(@STORM_HAVE_LIBARCHIVE@) # STORM_HAVE_LIBARCHIVE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this code not need the brew workaround?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh, it might actually need it.
| */ | ||
| Iterator& operator++(); | ||
|
|
||
| ArchiveReadEntry operator*() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return a reference to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ArchiveReadEntry essentially wraps around two (non-owning) pointers.
Returning a reference here has no real benefit and we would need to have an entity that "owns" ArchiveReadEntries.
Ahh, true. I did not read that paragraph carefully enough. Sorry :) |
…lue type from @value_type section
|
For future reference, I used the following script to test DRN import/export from CLI a little |
volkm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general. Thanks for all the work.
I have mostly minor comments which should hopefully be fast to modify.
| enum class CompressionMode { Default, None, Gzip, Xz }; | ||
|
|
||
| /*! | ||
| * @return The expression mode whose string representation matches the given input |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * @return The expression mode whose string representation matches the given input | |
| * @return The compression mode whose string representation matches the given input |
| * Sets the bits in the given bucket to the given value. | ||
| * @param bucketIndex The bucket. Bucket index i refers to the 64 bits with index i*64 to i*64+63. | ||
| * @param value The values to set. E.g., 1ull sets bit i*64 to 1 and all other bits in the bucket to 0 | ||
| * @note: bucketIndex must be below bucketCount(). Values for bitindices > size() are ignored |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * @note: bucketIndex must be below bucketCount(). Values for bitindices > size() are ignored | |
| * @note: bucketIndex must be below bucketCount(). Values for bitindices > bucketCount() are ignored |
| void setBucket(uint64_t bucketIndex, uint64_t value); | ||
|
|
||
| /*! | ||
| * Gets the bits in the given bucket to the given value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * Gets the bits in the given bucket to the given value. | |
| * Gets the bits in the given bucket. |
| * Gets the bits in the given bucket to the given value. | ||
| * @param bucketIndex The bucket. Bucket index i refers to the 64 bits with index i*64 to i*64+63. | ||
| * @return The values of the bits in the given bucket. | ||
| * @note: bucketIndex must be below bucketCount(). Values for bitindices > size() are always 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * @note: bucketIndex must be below bucketCount(). Values for bitindices > size() are always 0 | |
| * @note: bucketIndex must be below bucketCount(). Values for bitindices > bucketCount() are always 0 |
| * | ||
| * @return The number of buckets of the underlying storage. | ||
| */ | ||
| size_t bucketCount() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are moving the function, maybe move it after size()?
But not important.
| #include "storm/storage/BitVector.h" | ||
| #include "storm/utility/macros.h" | ||
|
|
||
| #include "storm/exceptions/NotSupportedException.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #include "storm/storage/BitVector.h" | |
| #include "storm/utility/macros.h" | |
| #include "storm/exceptions/NotSupportedException.h" | |
| #include "storm/exceptions/NotSupportedException.h" | |
| #include "storm/storage/BitVector.h" | |
| #include "storm/utility/macros.h" |
| } | ||
| #else | ||
| ArchiveWriter::ArchiveWriter(std::filesystem::path const&, CompressionMode const) { | ||
| throw storm::exceptions::NotSupportedException() << "Writing archives is not supported. Storm is compiled without LibArchive."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| throw storm::exceptions::NotSupportedException() << "Writing archives is not supported. Storm is compiled without LibArchive."; | |
| throw storm::exceptions::MissingLibraryException() << "Writing archives is not supported. Storm is compiled without LibArchive."; |
| // Free the entry metadata after we finish writing | ||
| archive_entry_free(entry); | ||
| #else | ||
| throw storm::exceptions::NotSupportedException() << "Writing archives is not supported. Storm is compiled without LibArchive."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| throw storm::exceptions::NotSupportedException() << "Writing archives is not supported. Storm is compiled without LibArchive."; | |
| throw storm::exceptions::MissingLibraryException() << "Writing archives is not supported. Storm is compiled without LibArchive."; |
| unset(LibArchive_BREW_HINT) | ||
| endif() | ||
| find_package(LibArchive QUIET) | ||
| if(TARGET LibArchive::LibArchive) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use LibArchive_FOUND?
| # following https://stackoverflow.com/a/76916842 | ||
| # Homebrew ships libarchive keg only, include dirs have to be set manually | ||
| execute_process( | ||
| COMMAND brew --prefix libarchive |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use the HINTS for find_package instead of setting the include directory? Alternatively, you could try setting LibArchive_ROOT.
Last idea could be to write a FindLibArchive.cmake which contains the homebrew workaround. This might then also work if we install Storm.
Storm can now be linked with libarchive as an optional dependency
New features:
--io:digitsoptionOnce this is merged, we need to adapt this page and add libarchive to the optional dependencies on the website.
Side note: I noted that the DRN page does not mention the@nr_choicessection.