Skip to content
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

gnupatch: 2.7.6 -> 2.7.6-unstable-2024-08-25 #337961

Draft
wants to merge 5 commits into
base: staging
Choose a base branch
from

Conversation

tie
Copy link
Member

@tie tie commented Aug 28, 2024

Description of changes

This change updates gnupatch to the latest Git commit.

Note that stdenv/linux uses fetchurlBoot as fetchurl for bootstrap and we need fetchFromSavannah (or rather fetchzip) that is not compatible with fetchurlBoot, so we override fetchzip to use fetchurl from the current package set instead of using fetchurlBoot. This shouldn’t be an issue on Darwin because curl is included in bootstrap files. FreeBSD stdenv seems to have curl in bootstrap files as well.

References:

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • 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.

@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Aug 28, 2024
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux-stdenv This PR causes stdenv to rebuild 8.has: package (new) This PR adds a new package labels Aug 28, 2024
@tie tie requested a review from emilazy August 28, 2024 19:00
@tie tie force-pushed the gnupatch-unstable branch from 8dad1b6 to 5b6341f Compare August 28, 2024 19:14
@emilazy
Copy link
Member

emilazy commented Aug 28, 2024

Thanks! I don’t feel entirely confident to review this gnarly stdenv bootstrap stuff but I’ll try to take a closer look soon.

Could we avoid some of the bootstrapping issues by just using fetchurl? I don’t know if we need fetchzip specifically for this.

@tie
Copy link
Member Author

tie commented Aug 28, 2024

Could we avoid some of the bootstrapping issues by just using fetchurl? I don’t know if we need fetchzip specifically for this.

If Git tarballs generated by Cgit are reproducible, then yes. Otherwise we can’t use fetchurlBoot because it hashes the file instead of archive contents. E.g. see https://github.blog/open-source/git/update-on-the-future-stability-of-source-code-archives-and-hashes

tie added 5 commits September 1, 2024 00:48
This changes openssl to use makeBinaryWrapper since makeWrapper uses
non-overridable runtimeShell that causes infinite recursion. That is,
fetchurl in pkgs/top-level/all-packages.nix is bootstrapped by
overriding dependencies to use stdenv.fetchurlBoot.

This change potentially allows us to drop curl from bootstrap files
for other platforms.
This change bootstraps fetchurl earlier in the pkgs/stdenv/linux stages
for use with other fetchers like fetchzip.
@tie tie force-pushed the gnupatch-unstable branch from 5b6341f to 6be2026 Compare August 31, 2024 22:30
@ofborg ofborg bot requested review from ulrikstrid and thillux September 1, 2024 00:47
Comment on lines +187 to +204
gnulibBootstrapHook = callPackage (
{
makeSetupHook,
gnulib,
autoconf,
automake,
}:
makeSetupHook {
name = "gnulib-bootstrap-hook";
propagatedBuildInputs = [
autoconf
automake
];
substitutions = {
inherit gnulib;
};
} ../build-support/setup-hooks/gnulib-bootstrap.sh
) { };
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be in pkgs/by-name/gn/gnulibBootstrapHook. I'd prefer that.

Copy link
Member Author

@tie tie Sep 5, 2024

Choose a reason for hiding this comment

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

Hm, I’d at least expect pkgs/by-name to be case-insensitive (or rather forcing all package names to be lower-case and erroring otherwise) since that’s a reasonable behavior given that macOS and Windows use case-insensitive filesystems by default. I’m not sure if that’s the case though.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have a check in nixpkgs-vet that names can't differ by case -- food and FooD are rejected. macOS and Windows are case insensitive but case-preserving, so we can use the filesystem for determining attrset names as pkgs/by-name does.

@@ -624,7 +633,7 @@ in

shell = cc.shell;

inherit (prevStage.stdenv) fetchurlBoot;
inherit fetchurlBoot;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this need to change?

Copy link
Member Author

Choose a reason for hiding this comment

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

No reason in particular, it was easier to remove all fetchurl/fetchurlBoot instances in stdenv/linux and rewrite it from scratch to when working on this PR, and I haven’t minimized the diff yet because some commits should be split into separate PRs (e.g. openssl stuff).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can merge this separate from this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I was planning to do this 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can merge this separate from this PR, and since it's a version bump, I'd prefer that.

Copy link
Member

Choose a reason for hiding this comment

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

Can we? I thought the change to python3Minimal would be load‐bearing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I’m not really sure if gnulib even uses/needs python3, i.e. by default gnulib-tool does a PATH lookup before using Python implementation (unless GNULIB_TOOL_IMPL is explicitly set).

https://github.com/coreutils/gnulib/blob/6e95321249ae6986c3df764a6e539f8b5be13948/gnulib-tool#L161

@philiptaron
Copy link
Contributor

https://github.com/NixOS/nixpkgs/actions/runs/10648732824/job/29518094549?pr=337961

This nixpkgs-vet error is unexpected to me. Taking a look.

@emilazy
Copy link
Member

emilazy commented Sep 4, 2024

I think the check was broken at some point? It might need a rebase.

@infinisil
Copy link
Member

The error is misleading, but the underlying problem seems to be that pkgs.gnupatch isn't actually pointing to the definition in pkgs/by-name, neither before nor after this PR:

nix-repl> builtins.unsafeGetAttrPos "gnupatch" pkgs
{
  column = 33;
  file = "/home/ncasil/src/nixpkgs/pkgs/stdenv/linux/default.nix";
  line = 679;
}

Not entirely sure why that happens, because both all-packages.nix and pkgs/by-name should override stdenv definitions:

toFix = lib.foldl' (lib.flip lib.extends) (self: {}) ([
stdenvBootstappingAndPlatforms
stdenvAdapters
trivialBuilders
splice
autoCalledPackages
allPackages
otherPackageSets

@tie
Copy link
Member Author

tie commented Sep 10, 2024

@infinisil, I assume this is because of the stdenv.overrides inherited from the previous stage (and stdenvOverrides taking precedence over allPackages)?

overrides = self: super: {
inherit (prevStage)
gzip bzip2 xz bash coreutils diffutils findutils gawk
gnused gnutar gnugrep gnupatch patchelf
attr acl zlib libunistring;
inherit (prevStage.gnugrep) pcre2;
${localSystem.libc} = getLibc prevStage;
# Hack: avoid libidn2.{bin,dev} referencing bootstrap tools. There's a logical cycle.
libidn2 = import ../../development/libraries/libidn2/no-bootstrap-reference.nix {
inherit lib;
inherit (prevStage) libidn2;
inherit (self) stdenv runCommandLocal patchelf libunistring;
};
gnumake = super.gnumake.override { inBootstrap = false; };
} // lib.optionalAttrs (super.stdenv.targetPlatform == localSystem) {
# Need to get rid of these when cross-compiling.
inherit (prevStage) binutils binutils-unwrapped;
gcc = cc;
};
};
})
# This "no-op" stage is just a place to put the assertions about stage5.
(prevStage:
# previous stage5 stdenv; see stage3 comment regarding gcc,
# which applies here as well.
assert isBuiltByNixpkgsCompiler prevStage.binutils-unwrapped;
assert isBuiltByNixpkgsCompiler prevStage.${localSystem.libc};
assert isBuiltByNixpkgsCompiler prevStage.gcc-unwrapped;
assert isBuiltByNixpkgsCompiler prevStage.coreutils;
assert isBuiltByNixpkgsCompiler prevStage.gnugrep;
assert isBuiltByNixpkgsCompiler prevStage.patchelf;
{ inherit (prevStage) config overlays stdenv; })

# stdenvOverrides is used to avoid having multiple of versions
# of certain dependencies that were used in bootstrapping the
# standard environment.
stdenvOverrides = self: super:
(super.stdenv.overrides or (_: _: {})) self super;

toFix = lib.foldl' (lib.flip lib.extends) (self: {}) ([
stdenvBootstappingAndPlatforms
stdenvAdapters
trivialBuilders
splice
autoCalledPackages
allPackages
otherPackageSets
aliases
configOverrides
] ++ overlays ++ [
stdenvOverrides ]);

This feels really weird to me, especially since the inherited (g)libc depends on xgcc-*-libgcc while other packages (that are not present in stdenv.overrides) are using regular gcc-*-libgcc. E.g.

$ nix path-info --recursive $(nix build --no-link --print-out-paths --file . bash^out) | fgrep libgcc
/nix/store/3kghvsns26scklmgi2f14787lzwrvqnf-xgcc-13.3.0-libgcc
nix path-info --recursive $(nix build --no-link --print-out-paths --file . coreutils^out) | fgrep libgcc
/nix/store/3kghvsns26scklmgi2f14787lzwrvqnf-xgcc-13.3.0-libgcc
/nix/store/vxk9halw286r11jabdyni93kkghhb9f5-gcc-13.3.0-libgcc

But perhaps that is a separate issue.

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: stdenv Standard environment 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 10.rebuild-linux-stdenv This PR causes stdenv to rebuild
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants