Skip to content

gettext, texinfo: Remove spurious xz #314670

Merged
Ericson2314 merged 2 commits intoNixOS:stagingfrom
rhelmot:freebsd-minimal3/xz-uses
May 27, 2024
Merged

gettext, texinfo: Remove spurious xz #314670
Ericson2314 merged 2 commits intoNixOS:stagingfrom
rhelmot:freebsd-minimal3/xz-uses

Conversation

@rhelmot
Copy link
Contributor

@rhelmot rhelmot commented May 25, 2024

Description of changes

part of #296581

If xz is overridden with a derivation that does not have an explicit bin output, these will fail. These changed instances are the ones impacted by the FreeBSD stdenv providing a bootstrap-files xz for early boot.

diffoscope indicates that the builds with and without xz are identical, so just remove.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
    • x86_64-freebsd
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels May 25, 2024
@rhelmot rhelmot force-pushed the freebsd-minimal3/xz-uses branch from 40c8cb6 to 19ccb97 Compare May 26, 2024 07:15
@isabelroses isabelroses added the 12.approvals: 1 This PR was reviewed and approved by one person. label May 26, 2024
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this automatically chosen here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to the other xz.bin this seems to have been introduced as part of #7701 in 2014:
4dccb22#diff-a05908b6eefb0d3630d5d06934fbfb4f0294382eeefef4438d9facb44a5bc291

Copy link
Member

@SuperSandro2000 SuperSandro2000 May 26, 2024

Choose a reason for hiding this comment

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

strictDeps is already set and --with-xz seems to be not properly checked and I think right now it is inneffective. We would probably need to move xz to buildInputs to properly work but it doesn't seem to do anything, at least nothing is linked against it.

Suggested change
(lib.getBin xz)

Copy link
Member

Choose a reason for hiding this comment

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

This seems wrong to me especially for cross compilation

Copy link
Contributor

Choose a reason for hiding this comment

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

Is your comment about xz.bin in the first place or about the change to lib.getBin xz?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I think @SuperSandro2000 means this should be a nativeBuildInput

Copy link
Member

Choose a reason for hiding this comment

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

Is your comment about xz.bin in the first place

yes

Copy link
Member

@SuperSandro2000 SuperSandro2000 May 26, 2024

Choose a reason for hiding this comment

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

We don't need that here anymore. At the time the tarball was in the xz format but now we have gz and strictDeps is already set.

I've also built the package with and without xz and diffoscope didn't find changes except the store path change.

Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

Please put the description in the commit messages (goes for many of the other PRs too).

I think it's OK to just leave the texinfo one as-is with the suspicious use of buildInputs as that's an orthogonal problem, but if you are feeling motivated you could add a strictDeps = true and see if nativeBuildInputs is better (simulating cross).

Comment on lines 54 to 55
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
xz
xz.bin
(lib.getBin xz)

I've run diffoscope on a gettext build with and without xz and booth are equal. I think we can just drop xz entirely from here.

and we can also drop the "--with-xz" arg in line 31.

rhelmot added 2 commits May 26, 2024 11:30
diffoscope indicates that the builds with and without xz are identical.
diffoscope indicates that the builds are the same with and without it.
@rhelmot rhelmot force-pushed the freebsd-minimal3/xz-uses branch from 19ccb97 to 9f481b3 Compare May 26, 2024 18:36
@rhelmot
Copy link
Contributor Author

rhelmot commented May 26, 2024

I have changed the commits as per feedback, removing both uses!

@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. labels May 26, 2024
@ofborg ofborg bot requested review from oxij, vrthra and zimbatm May 26, 2024 20:51
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels May 26, 2024
@SuperSandro2000 SuperSandro2000 changed the title xz: replace some usages of xz.bin with lib.getBin xz xz: remove spurious xz May 27, 2024
@Ericson2314 Ericson2314 changed the title xz: remove spurious xz gettext, texinfo: Remove spurious xz May 27, 2024
@Ericson2314 Ericson2314 changed the base branch from master to staging May 27, 2024 14:39
@Ericson2314 Ericson2314 merged commit 9e21798 into NixOS:staging May 27, 2024
@Ericson2314 Ericson2314 deleted the freebsd-minimal3/xz-uses branch May 27, 2024 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants