[liblzma] Update to version 5.6.2.#39024
Conversation
Validated by building the tools and successfully running them with `--help`.
|
Feature Usage test passed on |
|
@teo-tsirpanis: Thanks, I had to create the ticket for this new version (5.6.2), and I see that you have already done the PR, good job! cc: @carsten-grimm. Linked to:
Official XZ links: |
|
I happened to learn that vcpkg applies patches which I don't remember anyone submitting upstream (to me). I would like to know if the patches in vcpkg are still doing what they originally did and if something should be changed upstream.
|
Instead of making it platform dependent make an option for it and default that option dependening on platform. This way vcpkg does not need patching for it. vcpkg can use pc files on all platforms especially for meson and autotools based projects. vcpkg probably could build liblzma using autotools using msvc and thus generate the pc files automatically but CMake is simply the preferred build system within vcpkg.
If that is the case the patch can be removed.
Probably just a case of many ways to get the same output name using cmake. Needs testing if the patch can be removed. (but it probably can be removed) |
I had misunderstood that it wouldn't be useful with MSVC. vcpkg had been installing liblzma.pc with custom rules since 2020, years before liblzma.pc support was added to CMakeLists.txt. See: https://github.com/microsoft/vcpkg/blob/eb895b95aac6fd7485373702f29f508c42a180a0/ports/liblzma/portfile.cmake microsoft/vcpkg#39024 (comment)
|
The only platform where the I don't know if Autotools + MSVC works in XZ Utils; it's possible that something tiny is lacking at the XZ Utils side. I don't remember if anyone has ever tested it. CMake support was added initially to get rid of VS project files. Only later CMake support was expanded to non-MSVC use cases.
As far as I know, Windows needs nothing to enable threading support, thus |
|
The name patching and the pc file belong together: It makes the lib universally available via |
|
Before the linked fix one got If the intent is to get
then I see how it relates to So something needs to be fixed somewhere but I need to sleep now. ;-) |
This is a mess because liblzma DLL outside Cygwin and MSYS2 is liblzma.dll instead of lzma.dll to avoid a conflict with lzma.dll from LZMA SDK. On Cygwin the name was "liblzma-5.dll" while "cyglzma-5.dll" would have been correct (and match what Libtool produces). MSYS2 likely was broken too as it uses the "msys-" prefix. This change has no effect with MinGW-w64 because with that the "lib" prefix was correct already. With MSVC builds this is a small breaking change that requires developers to adjust the library name when linking against liblzma. The liblzma.dll name is kept as is but the import library and static library are now lzma.lib instead of liblzma.lib. This is helpful when using pkgconf because "pkgconf --msvc-syntax --libs liblzma" outputs "lzma.lib" (it's converted from "-llzma" in liblzma.pc). It would be easy to keep the liblzma.lib naming but the pkgconf compatibility seems worth it in the long run. The lzma.lib name is compatible with MinGW-w64 too as -llzma will find also lzma.lib. vcpkg had been patching CMakeLists.txt this way since 2022 but I learned this only recently. The reasoning for the patch makes sense, and while this is a small breaking change with MSVC, it seems like a decent compromise as it keeps the DLL name the same. 2022 patch in vcpkg: https://github.com/microsoft/vcpkg/blob/0707a17ecf1466d64cf1a3c1ee18c8ff02aadb2d/ports/liblzma/win_output_name.patch See the discussion: microsoft/vcpkg#39024 Thanks to Vincent Torri for confirming the naming issue on Cygwin.
|
I understand a few details better and made a few commits upstream. Here are updates to my earlier five questions and also two new ones:
I would be nice to know if the upstream changes are OK from vcpkg's point of view. The changes will be in the next stable release if there are no complaints. Thanks! The Autotools-based build has options to disable individual command line tools but the CMake-based build doesn't. Perhaps such options should be added upstream so that |
|
(Community feedback)
vcpkg doesn't want bundles, so the patch indicated a problem IMO. But I don't use iOS...
❤️
🚀
👍
getopt_long going into executables shouldn't be a barrier, in particular for an optional feature. It is also used by other ports. (And there is a port getopt-win32, but don't ask me about it is security and maintenance.)
For vcpkg, a single switch for tools would be enough for this port. It is fine to make a draft PR for a vcpkg CI run with a pre-release commit, to check if any further tweaks would be needed. That PR can be switched to the release when it is available. |
As long as the name of the generated binaries matches between versions removing the patch seems OK to me.
I think it should. Deciding to provide .pc files is a build system concern unrelated to what compiler or host one is on. It's true that That said, the port still works with the patches as described here; is it OK if we take this version that built successfully and removing patches if necessary can be done in a followup? |
|
So if I understand correctly, we can remove all patches except |
It looks like most of them can go away, yes. But we don't want to block contributors submitting good-faith updates on doing every cleanup thing, if that makes sense. If you want to remove them I think that would be nice though :) |
|
Note: @Larhzu is a primary maintainer of liblzma so we want to make sure they are happy. |
d099557 to
9f5f2d2
Compare
|
Yes I know. I updated the patches, hope it's right now. |
|
@BillyONeal: Of course. It is important to specify that I have contacted @Larhzu about the backdoor which has been discovered by @anarazel (a Microsoft dev), my update request of the "bad version", and about this new PR too. |
|
Yeah sorry, I was noting it for other vcpkg maintainers :) . Often we would be like 'Well taking the version update is more important than these improvements' I wanted to underline that we shouldn't do that this time. |
dg0yt
left a comment
There was a problem hiding this comment.
packages/liblzma_x64-mingw-dynamic/bin/liblzma.dll
packages/liblzma_x64-mingw-dynamic/lib/liblzma.dll.a
LGTM.
Thanks! Things look good to me now. In case you handle the next release:
If anything feels uncertain, feel free to ping me, although it's possible that I cannot help much with MSVC-specific issues.
I spotted two new bugs that, once known, were very simple to fix. There was a chance that some vcpkg users could have hit those issues and even reported them to vcpkg. Then time would be wasted in figuring out what's wrong and getting them fixed. My understanding is that the whole point of PR reviews is to catch this kind of errors before they get merged. I agree that cleaning up the unneeded harmless patches isn't so essential. However, cleaning them up makes it easier to review the packaging as a whole and increases the probability that real issues are spotted in PR reviews. It took a while for me to notice the two actual problems because the unneeded patches added to the amount of things to review. In any case it's not about making me happy. It's irrelevant who spotted the problems; they are worth fixing no matter who reports them. I know it can feel tedious for contributors but unfortunately packaging isn't just about bumping a version number if patches are being applied. Getting things fixed at upstream will make packager's job easier in the long run as fewer patches will be needed. Thanks for the explanations! I may look at |
|
Slightly off-topic but it's relevant to vcpkg: There are three ways to find liblzma in CMake:
The (1) and (2) are mentioned in vcpkg's Should (3) be the only recommended method by upstream and vcpkg some day in the future? It feels tempting to deprecate (2) because one cannot rely on its availability in a generic case. And (1) is a bit incomplete and I'm not sure how easy it is to improve it. (Perhaps it could use pkg-config if pkg-config is available?) If (2) is valuable, I don't mind keeping it supported as the code already exists in |
Nope even though pc files are great they don't correctly support multi config in cmake. vcpkg could fix that for itself however fixing upstream cmake is nearly impossible.
Don't do that. Upstream CMake wants to deprecate the internal find modules in favor for provided
Multi-Config support would be missing. The LZMA_API_STATIC for vcpkg needs to be baked into one of the headers any way since some consumers like msbuild doesn't know cmake nor pc files. (and there is probably the wild Makefile which also does not use pkg-config and hardcodes every linked library) |
|
That's valuable information. I will keep (2) in CMake. :-) It's understandable to want to deprecate the internal modules in upsream CMake. In case of liblzma, most installations don't have the *.cmake files though because Autotools has been used. So relying on *.cmake files from liblzma is not an option at the moment. (CMake support in XZ Utils was initially added for Windows use. CMake support has gotten close to feature parity with Autotools only in 2024 and it still needs some polishing.)
I looked at CMake source: The first one finds four files. So if my search pattern is good enough, only a few modules use The second command finds 14 files. For example, Now I think that if I change anything, it could be adding *.cmake files from other build systems somehow. Multi-Config isn't relevant in that case, I suppose. Maybe a simple wrapper that calls
It's fine to do that in vcpkg. I just feel it's not ideal as an upstream solution. One could build both static and shared libs and wish to share the same headers. There likely is no perfect solution. Thanks! |
|
In reply to #39024 (comment):
Old school. They are unaware of actual package configuration, which is in particular a problem for static linkage.
Modern CMake. CMake based projects can save everything which is known at build time. And CMake offers an advanced package search procedure when looking for the package later. Note that you don't need CMake to create CMake config package. You could do it as well from the autotools build. It is just some CMake code setting up variables and targets, and implementing version checks.
As stated, "Multi-Config support would be missing." So ideally you would add CMake config to the autotools build if you want a consistent experience. |
|
In reply to #39024 (comment):
Ideally XZ Utils would implement a
Well, most of these uses are only to determine the installation prefix. They ignore the actual flags and libs. Including the actual link library name etc. One of my favorite anti-patterns.
Really not the vcpkg pattern with its triplets - and with the MSVC world: same extension for static lib and import lib; different decorations when including headers. Autotools projects usually cares less about MSVC and typically build both types of libs. |
This could be something to study some day but I don't plan to focus on it in the near future.
I believe it already does provide compatibility with that spelling variation. It's just that in many cases the config files aren't present (Autotools build) and thus other packages cannot rely on them. To make things more complicated, there's a fairly strong wish from people to see Meson support. At the same time I have gotten wishes to keep Autotools support because Thanks! |
Meson. I expected that. The next system which you may love and hate at the same time. |
|
I think the ci was stuck |
|
Thank you very much for the update! |
I had misunderstood that it wouldn't be useful with MSVC. vcpkg had been installing liblzma.pc with custom rules since 2020, years before liblzma.pc support was added to CMakeLists.txt. See: https://github.com/microsoft/vcpkg/blob/eb895b95aac6fd7485373702f29f508c42a180a0/ports/liblzma/portfile.cmake microsoft/vcpkg#39024 (comment) (cherry picked from commit afa938e)
This is a mess because liblzma DLL outside Cygwin and MSYS2 is liblzma.dll instead of lzma.dll to avoid a conflict with lzma.dll from LZMA SDK. On Cygwin the name was "liblzma-5.dll" while "cyglzma-5.dll" would have been correct (and match what Libtool produces). MSYS2 likely was broken too as it uses the "msys-" prefix. This change has no effect with MinGW-w64 because with that the "lib" prefix was correct already. With MSVC builds this is a small breaking change that requires developers to adjust the library name when linking against liblzma. The liblzma.dll name is kept as is but the import library and static library are now lzma.lib instead of liblzma.lib. This is helpful when using pkgconf because "pkgconf --msvc-syntax --libs liblzma" outputs "lzma.lib" (it's converted from "-llzma" in liblzma.pc). It would be easy to keep the liblzma.lib naming but the pkgconf compatibility seems worth it in the long run. The lzma.lib name is compatible with MinGW-w64 too as -llzma will find also lzma.lib. vcpkg had been patching CMakeLists.txt this way since 2022 but I learned this only recently. The reasoning for the patch makes sense, and while this is a small breaking change with MSVC, it seems like a decent compromise as it keeps the DLL name the same. 2022 patch in vcpkg: https://github.com/microsoft/vcpkg/blob/0707a17ecf1466d64cf1a3c1ee18c8ff02aadb2d/ports/liblzma/win_output_name.patch See the discussion: microsoft/vcpkg#39024 Thanks to Vincent Torri for confirming the naming issue on Cygwin. (cherry picked from commit e0d6d05)
|
It has been updated to 5.6.3 by @c8ef: |
./vcpkg x-add-version --alland committing the result.This PR updates liblzma to the just-released version 5.6.2. I first copied the reverted port for 5.6.0, and updated versions.
I also enabled the tools feature on Windows; the tools seem to run fine there.