Skip to content
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

[minizip] Make BZip2 an optional feature #7282

Merged
merged 2 commits into from
Jul 18, 2019

Conversation

rknall
Copy link
Contributor

@rknall rknall commented Jul 16, 2019

If the BZip2 package has not been found, ensure that the library does
not assume it is. This would stop any build on targets where bzip2 is
not installed. This can either be the bzip2 package provided by vcpkg
or locally on the system.

Feature:

  • Allow optionally to enable bzip2 supprt

If the BZip2 package has not been found, ensure that the library does
not assume it is. This would stop any build on targets where bzip2 is
not installed. This can either be the bzip2 package provided by vcpkg
or locally on the system.

Feature:
- Allow optionally to enable bzip2 supprt
@rknall
Copy link
Contributor Author

rknall commented Jul 16, 2019

I apologize for resubmitting this patch. Had some fun with the git rebase

@cenit
Copy link
Contributor

cenit commented Jul 16, 2019

This can produce inconsistencies. What if you install bzip2 on your own and then minizip? Minizip would catch bzip2 and enable the feature without your knowledge (and vcpkg’s). Then you could remove easily bzip2, leaving a broken minizip behind you.
This feature requires a different approach imho

@@ -2,4 +2,8 @@ Source: minizip
Version: 1.2.11-4
Homepage: https://github.com/madler/zlib
Description: Zip compression library
Build-Depends: bzip2, zlib
Build-Depends: zlib
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The feature names are scoped different from the package names, so in cases like this the feature is typically given a name like bzip2 instead of bzip2support.

Making bzip2 a default feature would avoid breaking current users. I haven't used minzip so I don't know what most project would expect.

@Rastaban Rastaban self-assigned this Jul 18, 2019
@Rastaban
Copy link
Contributor

Thank you for the PR! You are off to a good start. Rather than making the find_package optional, add a new CMake option that can be passed in from the vcpkg_configure_cmake call in the portfile and is only set to off when the bzip feature is not used. That will make the behavior in the CI system more robust (we sometimes install everything and don't guarantee an order for ports without dependencies, so bzip2 may or may not be installed before). That should also resolve @cenit's concern. I can find you an example of another port that does that if it would be helpful, just let me know.

Also bump the version in the control file to 1.2.11-5, it helps with the vcpkg update command.

@Rastaban Rastaban changed the title [minizip] Update CMakeLists.txt [minizip] Make BZip2 an optional feature Jul 18, 2019
@rknall
Copy link
Contributor Author

rknall commented Jul 18, 2019

Thank you for the comments. Just as a background, we have included minizip in our Wireshark distribution (as of yesterday), but would prefer going the vcpkg route instead of building it ourselves. I do share the concerns of making a now required feature optional, and already also found a few corners where this breaks compatibility. I will try to make the required changes as stated by cenit and resubmit

Also catch dependency in the corresponding cmake files
@rknall
Copy link
Contributor Author

rknall commented Jul 18, 2019

To ensure backwards compatibility, I have added the bzip2 feature to both libkml and collada-dom. Not sure if they need it though, but this way they will be built as before.

@Rastaban
Copy link
Contributor

You can make an optional feature on by default by adding Default-Features: bzip2 to the CONTROL file. It would gives this behavior on install:

has default bzip2 feature
vcpkg install minzip

disables bzip2 feature and only installs the core
vcpkg install minzip[core]

I am willing to let it be an op-in feature for now, but if it causes issues then it will be turned into a default-feature in the future.

@Rastaban Rastaban merged commit bb163f5 into microsoft:master Jul 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants