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

Building with --features=gix-features/zlib-stock still pulls in libz-ng-sys #841

Closed
1 task done
carlocab opened this issue May 3, 2023 · 7 comments · Fixed by #842
Closed
1 task done

Building with --features=gix-features/zlib-stock still pulls in libz-ng-sys #841

carlocab opened this issue May 3, 2023 · 7 comments · Fixed by #842
Labels
acknowledged an issue is accepted as shortcoming to be fixed feedback requested

Comments

@carlocab
Copy link

carlocab commented May 3, 2023

Duplicates

  • I have searched the existing issues

Current behavior 😯

Doing

cargo install --features=gix-features/zlib-stock

from the repository root still builds libz-ng-sys. It also builds libz-sys, as requested, so it's not very clear which backend ends up being used.

Expected behavior 🤔

Cargo should build only libz-sys, and not libz-ng-sys.

Steps to reproduce 🕹

Run:

curl -L https://github.com/Byron/gitoxide/archive/refs/tags/v0.25.0.tar.gz | tar x
cd gitoxide-0.25.0
cargo install --features=gix-features/zlib-stock

Instead of cargo install, one can also do

cargo tree --features=gix-features/zlib-stock | grep libz

to see that multiple backends are built.

@carlocab
Copy link
Author

carlocab commented May 3, 2023

One workaround is to do

cargo install --no-default-features --features=gix-features/zlib-stock

but it's not clear at all how much this disables relative to the default configuration. Ideally it should be simple to choose features so that they are identical to the default max configuration except for the zlib-ng backend.

Another workaround is to replace zlib-ng in this line

max-performance = [ "max-performance-safe", "gix-features/zlib-ng", "fast-sha1" ]

with zlib-stock, which results in the desired behaviour from

cargo install --features=gix-features/zlib-stock

However, it would be nice to not have to hack up a Cargo.toml here.

For reference, this is related to Homebrew/homebrew-core#129719, where we're trying to package gitoxide for Homebrew.

@Byron
Copy link
Owner

Byron commented May 4, 2023

Thank you so much for reporting this and most importantly, provide a workaround. What's astonishing here is that I was not aware that it's possible to specify any features that are defined in dependencies of the crate. This makes cargo features so much more powerful!

With that said, the way features are currently organized it's hard to reproduce exactly what the default is, minus zlib-ng, + zlib-stock, but being able to specify it from the command-line might be a game-changer for future iterations of how features are exposed by the crate.

As for the actual issue described here, there is nothing that can be done as cargo features are additive, and the default choice happens to optimize for performance. However, I think it could make a safer choice, as to not choose anything people usually want to override. Thus a better choice might be something like max-pure and then build on top of that. Thinking about it, I don't see a way yet to make it usable by everyone, it's really just about trying to find a local minimum that serves most.

Maybe for now, you can maintain a patch file to make the changes directly where you need them? cargo build --features=gix-features/zlib-stock isn't recommended for the reasons you mention, it's unclear what you get and I don't see a way to fix that.

I will think about better ways to organize features, but can't know for sure I will manage to make significant improvements.

@Byron Byron added help wanted Extra attention is needed acknowledged an issue is accepted as shortcoming to be fixed labels May 4, 2023
@Byron Byron added feedback requested and removed help wanted Extra attention is needed labels May 4, 2023
@Byron
Copy link
Owner

Byron commented May 4, 2023

In the linked PR (and soon in main) you find the improvements needed more more easily build something like max but with difference choices for zlib or hashing. Please let me know if this works for you. Please find attached the rendered docs, those will be up on docs.rs when there is a new gitoxide release.

gitoxide - Rust.pdf - jump right to the Package Maintainers section.

Thank you

@Byron Byron closed this as completed in #842 May 4, 2023
@carlocab
Copy link
Author

carlocab commented May 5, 2023

This works great, thanks!

❯ cargo tree --no-default-features --features=max-control,gix-features/zlib-stock,gitoxide-core-blocking-client,http-client-curl | rg libz
│   │   │   │   │   │   ├── libz-sys v1.1.8
│   │   │   │   │   │   └── libz-sys v1.1.8 (*)

Will this be in your next release?

@Byron
Copy link
Owner

Byron commented May 5, 2023

Yes, it will be, and I think it's worth waiting as it will contain gix status or something like it.

@carlocab
Copy link
Author

carlocab commented May 5, 2023

What's astonishing here is that I was not aware that it's possible to specify any features that are defined in dependencies of the crate.

Yes, the --features flag of cargo is woefully underdocumented. The gix-features/zlib-stock syntax is mentioned here, but I don't think it really gives you a clear idea of how it works.

Yes, it will be, and I think it's worth waiting as it will contain gix status or something like it.

Great, thanks!

My only other request is for you to avoid changing these flags in future releases. We only have a small team of maintainers at Homebrew for ~6000 packages, so we rarely have the time to check updates carefully for changes to the build configuration. We often only notice that configurations need to change when packages fail loudly while building.

@Byron
Copy link
Owner

Byron commented May 5, 2023

My only other request is for you to avoid changing these flags in future releases.

I will do my best to keep them those working and won't change them lightheartedly. Fortunately it seems like these flags are stabilizing, especially after the recent push, as they seem to now fit their purpose pretty well.

Besides that, it's exciting to see gitoxide on homebrew, thanks for all your work :).

github-merge-queue bot pushed a commit to Homebrew/homebrew-core that referenced this issue May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acknowledged an issue is accepted as shortcoming to be fixed feedback requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants