Skip to content

[liblzma] update to version 5.6.0#37199

Merged
JavierMatosD merged 8 commits intomicrosoft:masterfrom
carsten-grimm:update-lzma-5.6.0
Mar 11, 2024
Merged

[liblzma] update to version 5.6.0#37199
JavierMatosD merged 8 commits intomicrosoft:masterfrom
carsten-grimm:update-lzma-5.6.0

Conversation

@carsten-grimm
Copy link
Copy Markdown
Contributor

@carsten-grimm carsten-grimm commented Mar 6, 2024

Fixes #37197.

  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

The update to version 5.6.0, includes the following changes

  • the patches were adapted for changes in the new version (I cannot check if the patch for iOS support was adapted correctly)
  • the new tools lzmadec and lzmainfo are handled in the same manner as the existing tools xz and xzdec
  • nls was disabled to sidestep an issue with installing the man pages
  • a new feature nls was added to enable native language support. This was necessary to handle a new optional dependency on gettext so that the existing tools feature continues to work.
  • nls support was not added, yet. See discussion below for details.

I have successfully built

  • liblzma:x86-windows,
  • liblzma:x64-windows,
  • liblzma:x64-linux,
  • liblzma[tools]:x64-linux,

Note that tools is not supported on windows.

Requested by @Neustradamus

EDIT 1: added nls feature
EDIT 2: removed nls feature again

@carsten-grimm carsten-grimm changed the title Update lzma 5.6.0 [liblzma] update to version 5.6.0 Mar 6, 2024
@Neustradamus
Copy link
Copy Markdown

@carsten-grimm: Thanks for your quick PR! :)

@LilyWangLL LilyWangLL self-assigned this Mar 7, 2024
@LilyWangLL LilyWangLL added the category:port-update The issue is with a library, which is requesting update new revision label Mar 7, 2024
@carsten-grimm carsten-grimm marked this pull request as ready for review March 7, 2024 13:53
LilyWangLL
LilyWangLL previously approved these changes Mar 8, 2024
-DBUILD_TESTING=OFF
-DCREATE_XZ_SYMLINKS=OFF
-DCREATE_LZMA_SYMLINKS=OFF
-DENABLE_NLS=OFF # avoid issues with installing language specific man files
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can't we disable the man files (which we don't want) instead of NLS (which should at least be an option IMO).

Copy link
Copy Markdown
Contributor Author

@carsten-grimm carsten-grimm Mar 8, 2024

Choose a reason for hiding this comment

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

I do not see an option for disabling the man files, but I can certainly try to write a patch that disables them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have a solution to re-enable nls. It requires gettext on linux to generate some .gmo files for the tools feature. I can add a cmake message to inform users that they need gettext if the tools feature is enabled.

Copy link
Copy Markdown
Contributor

@dg0yt dg0yt Mar 8, 2024

Choose a reason for hiding this comment

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

It became a pattern in autotools ports to have a non-default feature nls which depends on gettext-libintl and on host gettext[tools] (and sets config options).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Interesting, I will try adding the suggested nls feature tomorrow. I am reverting to a draft as that might lead to changes that should be re-reviewed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have added the nls feautre. Everything builds now and I can see the locale files in the package when building with the feature turned on. Let me know if I missed something.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The feature was removed again after the discussion below.

@carsten-grimm carsten-grimm marked this pull request as draft March 9, 2024 18:23
Copy link
Copy Markdown
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

I tested with mingw ... Maybe it is easier omit nls for now. It is only supported for NLS of the tools, but not for the lib. And I found that the vcpkg cmake wrapper for Intl has a bug which breaks builds for static linkage with external libintl when it depends on external libiconv (as in x64-mingw-static).

Comment on lines +22 to +29
"gettext",
{
"name": "gettext",
"host": true,
"features": [
"tools"
]
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
"gettext",
{
"name": "gettext",
"host": true,
"features": [
"tools"
]
}
{
"name": "gettext",
"host": true,
"features": [
"tools"
]
},
"gettext-libintl"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for looking into this @dg0yt .

Sorry, I am slighlty confused: Should I
(a) implement the change that you suggested or
(b) remove the nls feature?

If it is (a), I could mark mingw as not supported for nls.
If it is (b), I could remove the feature and turn ENABLE_NLS always off, as before.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Basically you can choose between a and b, both lead to a valid state for this port.
But reviewers might or might not complain in case of a. They won't complain about b.

And in the meantime, I try to fix/update gettext. At least you gave me a test port.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for clarifying. In this case, I will go with (b) to avoid controversy and keep the scope of the pull request to the update to 5.6.0.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done.

@carsten-grimm carsten-grimm marked this pull request as ready for review March 11, 2024 05:43
Copy link
Copy Markdown
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

LGTM but somebody else must approve.

@LilyWangLL LilyWangLL added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Mar 11, 2024
@JavierMatosD JavierMatosD merged commit 0ddcda3 into microsoft:master Mar 11, 2024
BillyONeal added a commit to BillyONeal/vcpkg that referenced this pull request Mar 29, 2024
BillyONeal added a commit that referenced this pull request Mar 30, 2024
Resolves #37839
Reverts #37199

See https://www.openwall.com/lists/oss-security/2024/03/29/4

Note that the version database is unmodified, only the baseline is
changed.

Because vcpkg builds liblzma from cmake sources downloaded from github
and this backdoor required modifications only present in the release
tarballs, it is our belief that vcpkg customers are not affected by this
problem. However, we are reverting this version out of an abundance of
caution as the threat actor clearly has broad access to liblzma
infrastructure, and because we believe customers will start flagging
this package by version as being a problem.
@Neustradamus
Copy link
Copy Markdown

Warning, some people attack me because I have requested the XZ update.
I am not linked to the XZ project.

@Neustradamus
Copy link
Copy Markdown

Neustradamus commented Mar 30, 2024

The official XZ team announcement is here:

Important to know: There is no problem with contributors here like @carsten-grimm.

But several people mix all because I have requested the XZ update in vcpkg.
I have received a lot of bad messages (private and public).
I have no link with XZ project, I follow only the project and do announcement or/and update requests.
I have requested 5.4.5 and 5.6.0 because there was only 5.4.4 in vcpkg.

@gowthamgts has participated on HN against me badly and I have commented on two places where he has commented (on my SCRAM request publications):

You can look here the original comment:

You can follow my announcements here:

The good point, people speak about SCRAM "Salted Challenge Response Authentication Mechanism" security ;)

Badly, some people or projects like only old unsecure mechanisms, some would like security improvements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:port-update The issue is with a library, which is requesting update new revision info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[liblzma] update to 5.6.0

5 participants