libzip: links with external libs found on host and new version

[prudhomm]

• closes libzip links with external libs found on host feelpp/spack#6

[wdconinc]

Since this is a CMakePackage AND an AutotoolsPackage, the variants must be supported in both or disabled in the AutotoolsPackage. This also requires the use of cmake_args in a builder class, see e.g. https://spack.readthedocs.io/en/latest/packaging_guide.html#multiple-build-systems.

[prudhomm]

@wdconinc I hope that I fixed libzip, it should now follow the multi-build system rules

[wdconinc]

One quick comment. I'll review when I'm in front of a full size screen.

[bernhardkaindl]

wdconinc I hope that I fixed libzip, it should now follow the multi-build system rules

No, you just moved the variants into build_system=cmake.
Wouter instead said that they should be in added to both build systems if they are added to one.

For example, this causes currently this error:

spack install libzip build_system=autotools ~bzip2
==> Error: failed to concretize `libzip~bzip2 build_system=autotools` for the following reasons:
     1. 'libzip' required multiple values for single-valued variant 'build_system'
     2. Cannot satisfy 'libzip@:1.3'
     3. Cannot satisfy 'libzip@:1.3'
        required because libzip variant build_system has value 'autotools' when @:1.3
          required because libzip~bzip2 build_system=autotools requested explicitly
     4. Cannot satisfy '[email protected]:' and 'libzip@:1.3
        required because libzip variant build_system has value 'autotools' when @:1.3
          required because libzip~bzip2 build_system=autotools requested explicitly
        required because libzip variant build_system has value 'cmake' when @1.4:
          required because libzip~bzip2 build_system=autotools requested explicitly

Please add an explanation why all these variants are suddenly needed and what is the benefit of enabling them now all at once and at the same time.

I asked them in #47240 before but as those changes don't even belong to #47240 but here, I add them here:

Observation: all of these appear to be new, and they are now all enabled by default. Before, libzip in spack did not use them:

• As all those variants are new, how libzip was just fine all the time in spack not needing those other variants.
• Are those variants also supported by the older versions of libzip that have versions in the spack recipe?
• Normally, I'd think that bzip2, lzma (likely also and zstd) should be quite fine as espezially bzip2 and lzma are very common. I just wonder how libzip did without them.
• At least lzma and bzip are small and fast to build and I guess the same for zstd.
• That's different for openssl, gnutls and mbedtls, one might not like them and disable them.
• What is the benefit of enabling multiple of those in addition to the dedicated compression libraries?
• What is their use in libzip?
• openssl is a very usual dependency of many spack packages, so it would be a default that would already be built more often than mbedtls or gnutls.

To make this easier, I think this PR should be split: If the new variants are not needed right now, I'd not add them now but instead focus on the new version and the include fixup that you added for it. Once that is ok, a later PR can deal with adding those variants properly.

PS: I only marked your PRs as draft because they are currently consuming GitLab CI resources that are not justified to be spent yet because basic requirements for an acceptable change are missing (described above, especially that the variants must be consistently work in both build systems if they are added to one).

[prudhomm]

@bernhardkaindl all the variant are enabled by default by libzip so if they are not installed by spack, libzip will have a look on the host system and use that. That's what happened to me and it created havock in my apps at linking stage because of conflicting libs. the previous versions of libzip spack package was buggy from this point of view and creates linkings issues if one of the libs libzip depends on is taken from the host.

I checked up to version 1.6 of libzip and all the variants are enabled.
here is 1.7.1
https://github.com/nih-at/libzip/blob/v1.7.1/CMakeLists.txt#L10

here is a copy paste of the CMakeLists.txt of 1.6

OPTION(ENABLE_GNUTLS "Enable use of GnuTLS" ON)
OPTION(ENABLE_MBEDTLS "Enable use of mbed TLS" ON)
OPTION(ENABLE_OPENSSL "Enable use of OpenSSL" ON)
OPTION(ENABLE_WINDOWS_CRYPTO "Enable use of Windows cryptography libraries" ON)

OPTION(ENABLE_BZIP2 "Enable use of BZip2" ON)
OPTION(ENABLE_LZMA "Enable use of LZMA" ON)

1.3 and 1.2 are deprecated, is that really necessary?

[prudhomm]

wdconinc I hope that I fixed libzip, it should now follow the multi-build system rules

No, you just moved the variants into build_system=cmake. Wouter instead said that they should be in added to both build systems if they are added to one.

For example, this causes currently this error:

spack install libzip build_system=autotools ~bzip2
==> Error: failed to concretize `libzip~bzip2 build_system=autotools` for the following reasons:
     1. 'libzip' required multiple values for single-valued variant 'build_system'
     2. Cannot satisfy 'libzip@:1.3'
     3. Cannot satisfy 'libzip@:1.3'
        required because libzip variant build_system has value 'autotools' when @:1.3
          required because libzip~bzip2 build_system=autotools requested explicitly
     4. Cannot satisfy '[email protected]:' and 'libzip@:1.3
        required because libzip variant build_system has value 'autotools' when @:1.3
          required because libzip~bzip2 build_system=autotools requested explicitly
        required because libzip variant build_system has value 'cmake' when @1.4:
          required because libzip~bzip2 build_system=autotools requested explicitly
```, 

they are deprecated versions I can have a look but the code is not even in github

Observation: all of these appear to be new, and they are now all enabled by default. Before, libzip in spack did not use them:

• As all those variants are new, how libzip was just fine all the time in spack not needing those other variants.

I am not a libzip developer, nor do I have the knowledge to develop it. However various libs I use, depends on libzip
the libzip package was broken in my opinion because it does not declare properly its relationship with the libs it depends upon and which are enabled by default in libzip.

• Are those variants also supported by the older versions of libzip that have versions in the spack recipe?

yes down to 1.6 (all of them enabled by default) except zstd which starts at 1.8
I haven't checked with the autotools, they are deprecated
I could check if it is really necessary

check out #47230 (comment) for more details

• Normally, I'd think that bzip2, lzma (likely also and zstd) should be quite fine as espezially bzip2 and lzma are very common. I just wonder how libzip did without them.

that's the thing, I don't understand how people never had an issue. Either spack was installing the other libs that libzip depends upon (implicitely) or they were found on the host or libzip didn't complain they were not available
so libzip had undefined behavior in my opinion or undefined compression support!

• At least lzma and bzip are small and fast to build and I guess the same for zstd.
• That's different for openssl, gnutls and mbedtls, one might not like them and disable them.
• What is the benefit of enabling multiple of those in addition to the dedicated compression libraries?
• What is their use in libzip?
• openssl is a very usual dependency of many spack packages, so it would be a default that would already be built more often than mbedtls or gnutls.

I am no expert in this area and I must admit that I don't have the time to look into that.
it was quite fast to get libzip up and running with the proper dependencies

if some compression algo should be disabled, someone more knowledgeable than me should take the decision.
I am just following libzip CMakeLists.txt default values and all libs are available in spack so I put them all.

To make this easier, I think this PR should be split: If the new variants are not needed right now, I'd not add them now but instead focus on the new version and the include fixup that you added for it. Once that is ok, a later PR can deal with adding those variants properly.

you know much better than me about all this.
I have exposed the buggy behavior. If someone has better knowledge how to do/fix it, he or she should do it I think

[prudhomm]

@bernhardkaindl @wdconinc libzip was not properly defined and libzip picked up what was available from spack or the host. I believe to avoid creating too many issues all the default options that libzip looks for should be enabled to avoid breakage.
but someone with spack knowledge and policy with respect to breaking apps should look into this

in my case libzip usage was already broken, that's why I tried to fix it.

[bernhardkaindl]

bernhardkaindl wdconinc libzip was not properly defined and libzip picked up what was available from spack or the host. I believe to avoid creating too many issues all the default options that libzip looks for should be enabled to avoid breakage.
but someone with spack knowledge and policy with respect to breaking apps should look into this

in my case libzip usage was already broken, that's why I tried to fix it.

Many thanks, I understand now! I am sorry that I was not able to see this before, but I get it now!

• As the old 1.2 and 1.3.2 versions are already deprecated in spack, it should be fine if you remove them now. (already deprecated versions are ok to remove, that is the one purpose of deprecating them)

• And it would make sense to now mark those older versions that don't support zstd as deprecated, so they can be removed next time or in the next cycle.

• Does is make sense to enable openssl, gnutls and mbedtls at the same time?

• If you do not need them, I'd think that it would be better to set default=False for them.

Other than that, in theory, after all versions that don't support build_system=cmake are removed, the build_system=autotools could be removed as well. Then you won't have the issue of having to support all variants in both build_systems.

[bernhardkaindl]

AFAICS, to support both build_systems, you have to remove this exclusive if for cmake and activate them for both (just remove the if and the indentation of the variants of course)

Suggested change

with when("build_system=cmake"):

Then, in the subclass for autotools, you have to pass those variants to configure similarily (but different of course) like you pass them in the cmake case to cmake.

[prudhomm]

@bernhardkaindl thank you for the feedback and your patience.
If I understand what you are saying it should be fine to get rid of autotools build_system because the versions associated are deprecated anyway. there are no particular policies in spack to make this kind of decision ? (IMO I am perfectly fine with it but there might be user who feel differently)

regarding the default variants, I think it does not depend on me but rather on the expected behavior of libzip and libzip up to now has been defined with all the variants on implicitly via libzip cmake. Shouldn't we have them on by default ?

[bernhardkaindl]

Disclaimer: I'm not speaking for spack, I'm just an independent SW developer who was volunteering to review PRs, please do not reflect this reply in spack itself:

@bernhardkaindl thank you for the feedback and your patience.
If I understand what you are saying it should be fine to get rid of autotools build_system because the versions associated are deprecated anyway. there are no particular policies in spack to make this kind of decision ? (IMO I am perfectly fine with it but there might be user who feel differently)

Hi @prudhomm, thank you for your explanation and patience as well! There is a policy that is in general against removing older versions, but it has been said if I understood the leadership correctly, that the 1st priority is not not make pull requests harder to pass than they need to be. Deprecating obsolete versions is part of that, and deprecated versions also then are not the primary focus of a PR anymore. And already deprecated versions are allowed to be removed. If a build system was only used by deprecated versions, that would include this build system. I'm only aware of one limitation regarding the build system: That is if the package is critical to building base packages needed to build e.g. cmake, of course, one could not remove the build system that is needed to build cmake without having cmake already. But libzip does not appear to be such a core package, AFAICS. git grep libzip shows qgis, netcdf-c, cntk, and fmi4cpp, and those are as far as I can see not core packages that are needed to build something central like cmake. As the build_system used to build a package is not something that should affect the dependent packages, it should be fine to make that change.

regarding the default variants, I think it does not depend on me but rather on the expected behavior of libzip and libzip up to now has been defined with all the variants on implicitly via libzip cmake. Shouldn't we have them on by default ?

Understood. Yep agreed. As I see that libzip is only direclty used by these packages and their dependencies possibly indirectly, I now understand that there is no need to make libzip small, or have as few dependencies as possible. However, one good sanity check, especially also with regards to any bigger change would be to build the dependents.

You can also get a confirmation of the dependents using spack dependents libzip. That also lists the packages that I found using git grep libzip. Whatever change you make, be sure to essentially run the equivalent of:

for package in $(spack dependents libzip); do spack install $package || break ;done

@prudhomm A final unfortunate comment: Because I reviewed (apparently) hundreds of PRs in the last weeks, I cannot continue to keep that up as a hobby. I have to unsubscribe from this PR and refer to other reviewers, Wouter was so kind to also look at this PR before, so you may ask him first to continue your review once you make the PR consistent with his remarks (if there are multiple build systems, both must be supported with the variants you add).

I see two ways to do that: Either implement applying the checks for satisfies for the cmake and autotools systems. If you can remove the deprecated versions. This allows you to remove the autotools system. Then you only need cmake, which of course makes things a lot simpler. And would be better: Simpler is also better - easier to maintain. But you should check that the dependents still work with the newer versions. I know that's a lot of work. It does not have to be done all in one PR, but as Wouter commented the PR should be consistent in itself regarding the variants.

I'd say that you should make the adjustments to make the PR consistent your call, you likely know best as you are the first to open a PR to fix these issues with libzip. So you can assume that by that knowledge, you also can assume that you know what's best going forward. Just explain that like you did for me, and as far as that be understood, I think that you should be ok.

Of course, testing that the dependents still build/work with the new cmake-only versions is likely an especially good idea.

As a last resort in case of lack of review or guidance join http://spackpm.slack.com, or open a discussion on https://github.com/spack/spack/discussions. Of course, you can mention me again, but I have to apologize that I have to wind down. 580 PRs were merged/closed last month, so that's a lot, so maybe spack can ways to scale PR review. Maybe can get some ChatGPT going to answer questions, that would be great! ;)

Disclaimer: I'm not speaking for spack, I'm just an independent SW developer who was volunteering to review PRs, please do not reflect this reply in spack itself!

PPS: Because I gave a review with the changes requested flag: To allow other reviewers to take over, I've to set the "approve" flag for this review to unblock other reviewers from reviewing/merging this PR.

[prudhomm]

@bernhardkaindl thank you for your very detailed work and review

[bernhardkaindl]

@prudhomm style prechecks in GitHub CI failed:

ci / prechecks / style (pull_request) Failing after 52s

Check its logs, likely a formatting failure. If that's the case, just run black on the recipe to fix formatting to be blacked.

Please re-request the review once all blocking issues are fixed.

I suggest fixing the review comments with current versions, to remove already deprecated versions and, (in case that applies) you can also deprecate versions that are obsolete and may no longer be built easily.

Please re-request review(s) (or mention reviewers) when you fixed CI, fixed comments and tested.