Migrate from unmaintained xz2 to liblzma

[xbjfk]

Hello,
It seems xz2 is unmaintained and the maintainer has stepped down: alexcrichton/xz2-rs#128 (comment). https://github.com/Portable-Network-Archive/liblzma-rs seems to be the replacement, including a newer version of xz, wasm support and test updates.
I've kept the module called xz2 for now. All tests still pass.

[NobodyXu]

Hmmm it seems that the fork is done about 10 months ago, I'm worried about if the maintainer is malicious just like the one in xz-utils.

I suggest that to add it as a separate feature lzma-new, xz-new, people who trust the new maintainer can opt-in to it, while other remains on xz-rs which is outdated but trusted.

If eventually there's enough evidence that the new fork is trustworthy then we'd switch the existing feature to use the fork.

cc @Nemo157 @robjtede

[xbjfk]

Sounds good to me, I do recognize the irony in this being for xz specifically :)
That should also make migration to then new name easier while not breaking anything - potentially calling the new module LibLzma and LibLzmaXz?

[NobodyXu]

potentially calling the new module LibLzma and LibLzmaXz?

I prefer lower-kerbab-case, otherwise it looks good to me

[xbjfk]

I prefer lower-kerbab-case, otherwise it looks good to me
Oh, I meant the modules, not the features

[NobodyXu]

Oh, I meant the modules, not the features

For modules, I think lower_case makes sense

[xbjfk]

Hmmm, it looks like having them added concurrently won't work, even if neither are enabled, since they both link lzma :(.
If you don't have any ideas, I'm fine with just using [patch].

[NobodyXu]

Hmmm, it looks like having them added concurrently won't work

If both of them are enabled, then we can default to using the fork.

The API should not be changed, as only the underlying bindings are swapped

[xbjfk]

Yes, I was attempting to do that, but it seems cargo will refuse to have both of them as a dependency (even optional?) because of the links field (doc):

The links field is used to ensure only one copy of a native library is linked into a binary. The resolver will attempt to find a graph where there is only one instance of each links name. If it is unable to find a graph that satisfies that constraint, it will return an error.

[NobodyXu]

Ohh right I forgot that😅
That's unfortunate, so maybe we can guard the fork against a custom cfg cfg(liblzma_fork)?

That'd allow us to only enable one of the dependency, without introducing new features.

[robjtede]

Agreed, I'm apprehensive to swap it out completely, but a cfg-based approach to the fork would be acceptable.

[NobodyXu]

cc @xbjfk We discussed about this and we think using cfg(async_compression_unstable_liblzma_fork) to guard against the new fork makes sense.

That'd allow us to only pull one version of it, and remove it in the future without breaking backwards compatibility.

[xbjfk]

@NobodyXu thanks, I agree and think that is the best way forward.
I'm still confused on how to make liblzma an optional dependency and satisfy cargo - I'll make my PR a draft so that we can test things out, and push my failed attempt at getting them to both be features.

[NobodyXu]

I'm still confused on how to satisfy cargo

I think you can do

[target.'cfg(not(async_compression_unstable_liblzma_fork))'.dependencies]
xz = ...

[target.'cfg(async_compression_unstable_liblzma_fork)'.dependencies]
xz-fork = ...

[xbjfk]

I think you can do

Thanks!

Unfortunately, cargo seems to still complain about the linkage :( (see: rust-lang/cargo#5969 (comment)). I've just pushed my code with the cfg option.
Manually adding/removing a dependency seems to work however, and all tests still pass.

[NobodyXu]

@xbjfk I think maybe we should open an issue in rust-lang/cargo?

It seems that the linkage - the dependency resolution is done before cfg selection?

It's originally intended for cfg target expression, so I kind of understand why it doesn't work, but I believe this can be supported in cargo.

[xbjfk]

Yeah, I agree.
This does seem simpler for cargo to support than linkage conflicts between optional dependencies.