-
Notifications
You must be signed in to change notification settings - Fork 7.2k
[unarr] add new package #47734
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
[unarr] add new package #47734
Conversation
JavierMatosD
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.
Is this library vending 7z?
https://github.com/selmf/unarr/blob/master/lzmasdk/7z.h
ports/unarr/portfile.cmake
Outdated
| vcpkg_check_features(OUT_FEATURE_OPTIONS FEATURE_OPTIONS | ||
| FEATURES | ||
| "enable-7z" ENABLE_7Z | ||
| "zlib-crc32" USE_ZLIB_CRC |
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.
I don't think this should be a feature since you're unconditionally depending on zlib. Unless I'm missing something?
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.
OK, I will keep this option ON at any case.
|
Dunno maybe Windows OS has some issues with 7zip port? |
|
Vcpkg tool bug, #47757 (comment). |
|
Thank you for your explanation, could you notify when storm would calm down and vcpkg would work? Thanks. |
|
microsoft/vcpkg-tool#1817 bug fix |
|
It looks like the failures are now "real" |
What would be insight for this case? |
If upstream doesn't care about supporting shared libraries, |
|
Unfortunately there are more bits that seem to have hard coded dependencies on the vendored lzmasdk. Examples: https://github.com/selmf/unarr/blob/1df8ab3d281409e9fe6bed8bf485976bb47f5bef/zip/zip.h#L21 I'm not sure it's going to be easy to devendor this thing :( |
dg0yt
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.
Just a quick look.
ports/unarr/debundle-7zip.patch
Outdated
| target_compile_definitions(unarr PRIVATE -DHAVE_BZIP2) | ||
| # Bzip2 upstream does not supply a .pc file. Add it to Libs.private. | ||
| - set(PROJECT_LIBS_PRIVATE "-I${BZIP2_INCLUDE_DIRS} -l${BZIP2_LIBRARIES}") | ||
| + set(PROJECT_LIBS_PRIVATE "${PROJECT_LIBS_PRIVATE} -I${BZIP2_INCLUDE_DIRS} -l${BZIP2_LIBRARIES}") |
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.
Not your bug, but: -I doesn't belong to pkg-config's Libs.private.
…for external 7zip codecs
Co-authored-by: Kai Pastor <[email protected]>
Co-authored-by: Kai Pastor <[email protected]>
Co-authored-by: Kai Pastor <[email protected]>
I think nor unarr nor 7zip intend to support Windows OS shared library build. |
7zip is fine. The 7zip build in vcpkg uses CMake, but with pristine |
Well I will place supports for |
ports/unarr/vcpkg.json
Outdated
| "description": "A decompression library for rar, tar, zip and 7z archives.", | ||
| "homepage": "https://github.com/selmf/unarr", | ||
| "license": "LGPL-3.0-only", | ||
| "supports": "!(windows & !static)", |
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.
Unfortunately:
!(windows & !static)!windows | !static(DeMorgan)
excludes every triplet we have so this doesn't pass the 'tested on at least one official triplet' requirement we need to put in the curated registry.
Did you intend !(windows & static)?
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.
Isn't it simply "SUPPORTS: NOT (WINDOWS & DYNAMIC)"?
* `!(windows & !static)` * `!windows | !static` (DeMorgan)
!(windows & !static)!windows | !!static!windows | static
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.
I intended to skip shared library builds for Windows OS, but keep everything else.
Is it "supports": "!windows | static", by any chance?
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.
Isn't it simply "SUPPORTS: NOT (WINDOWS & DYNAMIC)"?
I am an idiot, sorry :(
Is there difference between:
if(VCPKG_TARGET_IS_WINDOWS)
vcpkg_check_linkage(ONLY_STATIC_LIBRARY)
endif()? |
Yes. |
BillyONeal
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.
Thanks for the new port, for devendoring stuff, and sorry for the confusion. Thanks also @dg0yt for pointing out where Bill was a dum dum.
The first is checked by vcpkg tool when making an installation plan. Regardless of per-port triplet customization. The second is checked by CMake when vcpkg gets to actually building the port. Subject to per-port triplet customization. |
find_packagecalls are REQUIRED, are satisfied byvcpkg.json's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx.vcpkg.jsonmatches what upstream says.vcpkg.jsonmatches what upstream says../vcpkg x-add-version --alland committing the result.https://repology.org/project/unarr/versions