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

minizip2 2.10.4 #66598

Closed
wants to merge 4 commits into from
Closed

Conversation

carlocab
Copy link
Member

@carlocab carlocab commented Dec 9, 2020

Created with brew bump-formula-pr.

@carlocab
Copy link
Member Author

carlocab commented Dec 9, 2020

Build error:

The formula built, but is not symlinked into /usr/local
Could not symlink lib/libzstd.a
Target /usr/local/lib/libzstd.a
is a symlink belonging to zstd. You can unlink it:
  brew unlink zstd

It seems to conflict with zstd, but also depends_on "zstd"? Not really sure how to fix this.

I can turn off zstd support, but given that it's a default option, not sure that's desirable.

@chenrui333 chenrui333 added the build failure CI fails while building the software label Dec 9, 2020
@nmoinvaz
Copy link

nmoinvaz commented Dec 10, 2020

It appears to be looking for the libzstd.a in the source directory. I have recently changed the 3rd-party libs to build into whatever directory FetchContent creates.

@carlocab
Copy link
Member Author

Thanks for looking at it.

I'm not sure that's the problem. The problem is that it's found a libzstd.a in the install directory, which it wants to symlink into /usr/local/lib. However, it can't do that, because there is already a /usr/local/lib/libzstd.a (from zstd) so it doesn't know what to do.

@carlocab
Copy link
Member Author

Is the libzstd.a just a duplicate of the library that comes with zstd?

If it is I can just delete it and I think the build will proceed just fine.

@nmoinvaz
Copy link

nmoinvaz commented Dec 10, 2020

If it can't find libzstd on the machine it will clone the zstd repository and build it, so it is the built zstd library.

@carlocab
Copy link
Member Author

Ah, I see. Ok. I'll have to look at helping it find the existing zstd library then. Any tips on this would be appreciated, but I imagine I'll figure it out eventually.

@nmoinvaz
Copy link

It might be possible to add the BINARY_DIR to the FetchContent_Declare calls in the CMakeLists.txt. It looks like Homebrew is just looking for the file in a particular directory when it goes to package it up.

@carlocab
Copy link
Member Author

It looks like you have cmake set up to use pkg-config to look for libraries. I'll make pkg-config available to the build environment and see if that helps.

Build scripts don't seem to be able to find zstd without it, and so
builds duplicate libraries in the process.
It needs this for liblzma. It will attempt to use the master branch of
the liblzma repo otherwise.
@carlocab
Copy link
Member Author

carlocab commented Dec 10, 2020

Ok, the zstd problem was fixed in the last test run. But it seems to have failed the post-install test. I'm guessing that's because it's also rolled its own untested lzma library.

I've provided a tested lzma library as a dependency, so that should hopefully fix things.

Thanks again for the assist, @nmoinvaz. I have to say though -- cloning repos from their master branch to produce libraries that you expect to be there but aren't doesn't seem like a great practice...

@carlocab
Copy link
Member Author

@chenrui333 build failure should be fixed now.

@nmoinvaz
Copy link

I agree, it only does that when it does not find the the library installed on the system. It can also be disabled through -DZSTD_FORCE_FETCH=OFF but it is on by default.

@carlocab
Copy link
Member Author

It seems to be off by default here:

https://github.com/nmoinvaz/minizip/blob/318e848843667f982f81dbcb72334e415ba482a5/CMakeLists.txt#L34

It even looks like it does the opposite of what the name suggests (though the description does match). It's also not documented.

This behaviour has prevented two version bumps here at Homebrew: #64762, #63653

It would be useful for package managers like Homebrew if there were an option to turn off all fetching behaviour. If this option were the default, even better.

Apologies for the litany of complaints: I don't have a lot of experience with your fork of minizip but it seems like you've done great work with it. Just thought the feedback could be useful for when you think of things to improve.

@chenrui333 chenrui333 removed the build failure CI fails while building the software label Dec 10, 2020
Copy link
Member

@chenrui333 chenrui333 left a comment

Choose a reason for hiding this comment

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

nice work!

@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

@carlocab carlocab deleted the bump-minizip2-2.10.4 branch December 10, 2020 05:17
@nmoinvaz
Copy link

@carlocab if you can create an issue in minizip project then when I am working on it again next I can take a look. Thanks.

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.

None yet

4 participants