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

Avoid optionaly depends on zip for NCZarr #2592

Merged
merged 3 commits into from
Feb 27, 2023

Conversation

mwestphal
Copy link
Contributor

No description provided.

@mwestphal mwestphal requested a review from WardF as a code owner January 19, 2023 08:37
@CLAassistant
Copy link

CLAassistant commented Jan 19, 2023

CLA assistant check
All committers have signed the CLA.

@WardF
Copy link
Member

WardF commented Jan 19, 2023

Thanks for this; we'll need to make the appropriate changes in configure.ac as well, to cover our bases for both build systems we support, but this looks good, thanks!

@mwestphal
Copy link
Contributor Author

mwestphal commented Jan 26, 2023

@WardF It looks like I need to sign a CLA, however, the CLA in the link is for an individual. Do you have a CLA for an entity that my company could sign ?

@WardF
Copy link
Member

WardF commented Jan 26, 2023

@dopplershift Do we have a CLA for entities?

@WardF
Copy link
Member

WardF commented Jan 26, 2023

@mwestphal Thanks, that's a good question, and the first time (to my knowledge) we've been asked. Stand by while I get an answer for you :)

@dopplershift
Copy link
Member

Here's our entity CLA.

@WardF
Copy link
Member

WardF commented Jan 26, 2023

Thanks @dopplershift ! @mwestphal, the link to our entity CLA has been linked, and again, here.

@mwestphal
Copy link
Contributor Author

Here is the signed CLA, waiting for your signature.
CLA-UCAR.docx.pdf

@WardF
Copy link
Member

WardF commented Feb 27, 2023

Thanks! Reviewing the PR's now that I've got a lingering issue taken care of and need to get caught up on PR's. @dopplershift, the signed organizational CLA was provided above. What's our next step?

@WardF WardF self-assigned this Feb 27, 2023
@WardF WardF added this to the 4.9.2 milestone Feb 27, 2023
@dopplershift dopplershift reopened this Feb 27, 2023
Copy link
Collaborator

@DennisHeimbigner DennisHeimbigner left a comment

Choose a reason for hiding this comment

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

It occurs to me that this change is not what I would expect.
The case I am concerned with is:

  1. ENABLE_NCZARR_ZIP is on (which is the default)
  2. libzip is not available.

The current code makes libzip required in this case, which I assume will cause
an error. I think that is inconsistent with our current practice.
It probably should remove the required flag and if the find_package(zip)
fails, then report a warning, but do not cause the build to fail.

@mwestphal
Copy link
Contributor Author

This is indeed my intent. If a user require an option but the project can't provide it, an error is expected imo. But I'll let you the judge of it.

@DennisHeimbigner
Copy link
Collaborator

But the problem is that the naive user will see his build fail
because libzip is not installed even though he said nothing
about zip in his command line options. Remember, your code
enabled zip by default. To get the proper behavior, you have
to make zip disabled by default.

@mwestphal
Copy link
Contributor Author

that is fair to turn it off by default imo.

@WardF WardF merged commit 96c41b1 into Unidata:main Feb 27, 2023
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.

5 participants