Skip to content

chore: add uncheck-liblzma-version feature #342

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

Closed

Conversation

eatradish
Copy link

@eatradish eatradish commented Apr 14, 2025

liblzma-rs requires a higher version of liblzma to allow dynamic linking by default (see Portable-Network-Archive/liblzma-rs#173 for details), This feature will allow any version of liblzma on the system to use the dynamic linking

liblzma-rs requires a higher version of liblzma to allow dynamic linking
by default (see Portable-Network-Archive/liblzma-rs#173 for details),
This feature will allow any version of liblzma on the system to use the dynamic linking.
@eatradish eatradish force-pushed the add-uncheck-liblzma-version branch from e595a80 to 01b028d Compare April 14, 2025 21:02
Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

Thank you, I wonder if adding another feature is necessary?

You could add a dependency to liblzma in your project and enable that feature directly

@eatradish
Copy link
Author

eatradish commented Apr 15, 2025

Thank you, I wonder if adding another feature is necessary?

You could add a dependency to liblzma in your project and enable that feature directly

Hmm, well I'm not sure if I should add it or not

@eatradish
Copy link
Author

But I have a question, if I define a version of liblzma-rs that doesn't match the version that async-compression depends on, will the feature be overwritten?

@NobodyXu
Copy link
Collaborator

But I have a question, if I define a version of liblzma-rs that doesn't match the version that async-compression depends on, will the feature be overwritten?

No you would keep it in sync

@robjtede
Copy link
Member

robjtede commented Apr 15, 2025

I don't think we should provide proxy features for any of the codecs, you're able to enable these yourself, by adding this to your manifest:

liblzma = { version = "0.4.1", features = ["uncheck_liblzma_version"] }

@robjtede robjtede closed this Apr 15, 2025
Copy link

codecov bot commented Apr 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (a642a55) to head (01b028d).

Additional details and impacted files
@@     Coverage Diff     @@
##   main   #342   +/-   ##
===========================
===========================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@MingcongBai
Copy link

MingcongBai commented Apr 16, 2025

I don't think we should provide proxy features for any of the codecs, you're able to enable these yourself, by adding this to your manifest:

liblzma = { version = "0.4.1", features = ["uncheck_liblzma_version"] }

The only thing though is that it causes difficulty for building Rust programs on older platforms (say, Ubuntu 22.04) - by our testing, liblzma before and after 5.6 is not perfectly compatible... To quote the original issue on the liblzma-rs end:

However, with Ubuntu 22.04, oma built with statically linked xz/liblzma >= 5.6 would cause data errors during decompression (when xz(1) is called) - there seems to be some strange ABI incompatibilities between liblzma 5.2 and liblzma >= 5.6.

The approach you noted can also be quite tedious - say when async-compression's dependency is updated a liblzma version that differs from what is overriden in Cargo.toml, then the feature specification would then be invalidated (please do correct me if I'm wrong).

@eatradish eatradish deleted the add-uncheck-liblzma-version branch April 16, 2025 06:39
@mokurin000
Copy link

The approach you noted can also be quite tedious - say when async-compression's dependency is updated a liblzma version that differs from what is overriden in Cargo.toml, then the feature specification would then be invalidated (please do correct me if I'm wrong).

It's not only invalidated, but the whole build process will fail fast. cargo would say:

the package `liblzma-sys` links to the native library `lzma`, but it conflicts with a previous package which links to `lzma` as well:
package `liblzma-sys v0.4.3`
    ... which satisfies dependency `liblzma-sys = "^0.4.3"` (locked to 0.4.3) of package `liblzma v0.4.1`
    ... which satisfies dependency `liblzma = "^0"` (locked to 0.4.1) of package `testbuild v0.1.0 (/data/data/com.termux/files/home/testbuild)`
Only one package in the dependency graph may specify the same links value. This helps ensure that only one copy of a native library is linked in the final binary. Try to adjust your dependencies so that only one package uses the `links = "lzma"` value. For more information, see https://doc.rust-lang.org/cargo/reference/resolver.html#links.

But we could workaround things by simply not specifying version of liblzma.

[dependencies]
async-compression = { version = "0.4.22", features = ["lzma",] }
liblzma = { version = "0", features = [] }

@NobodyXu
Copy link
Collaborator

But we could workaround things by simply not specifying version of liblzma.

That's a new idea I didn't think of!

I think we can also have wildcard in version requirements, so

liblzma = { version = "*", features = [] }

Might work?

@robjtede
Copy link
Member

It's dangerous for async-compression to use * versions, and I usually advise to avoid them entirely.

Aside from the case with liblzma where multiple native libraries conflict, detecting duplicate packages generally is very easy with Cargo:

cargo tree -i liblzma

... will error if there are multiple liblzma in the dep tree.

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