Skip to content

Revert obsolete aarch64 musl hacks #283825

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.

Sign up for GitHub

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

Merged
alyssais merged 4 commits into from
Jan 25, 2024
Merged

Revert obsolete aarch64 musl hacks #283825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f389f9b
Revert "diffutils: disable tests on aarch64 musl"
alyssais Jan 25, 2024
ee809b2
Revert "pkgsMusl.coreutils: fix build on aarch64"
alyssais Jan 25, 2024
7cdb1c7
Revert "patchelf: use 0.13.x on aarch64+musl"
alyssais Jan 25, 2024
9e2a8f2
Revert "patchelf_0_13: init at 0.13.1"
alyssais Jan 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 0 additions & 23 deletions pkgs/development/tools/misc/patchelf/0.13.nix
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions pkgs/tools/misc/coreutils/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,10 +129,8 @@ stdenv.mkDerivation rec {
# Darwin (http://article.gmane.org/gmane.comp.gnu.core-utils.bugs/19351),
# and {Open,Free}BSD.
# With non-standard storeDir: https://github.com/NixOS/nix/issues/512
# On aarch64+musl, test-init.sh fails due to a segfault in diff.
doCheck = (!isCross)
&& (stdenv.hostPlatform.libc == "glibc" || stdenv.hostPlatform.libc == "musl")
&& !(stdenv.hostPlatform.libc == "musl" && stdenv.hostPlatform.isAarch64)
&& !stdenv.isAarch32;

# Prevents attempts of running 'help2man' on cross-built binaries.
Expand Down
2 changes: 1 addition & 1 deletion pkgs/tools/text/diffutils/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ stdenv.mkDerivation rec {
lib.optional (coreutils != null) "PR_PROGRAM=${coreutils}/bin/pr"
++ lib.optional (stdenv.buildPlatform != stdenv.hostPlatform) "gl_cv_func_getopt_gnu=yes";

doCheck = !(stdenv.buildPlatform.isAarch64 && stdenv.buildPlatform.isMusl);
doCheck = true;

meta = with lib; {
homepage = "https://www.gnu.org/software/diffutils/diffutils.html";
Expand Down
1 change: 1 addition & 0 deletions pkgs/top-level/aliases.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -795,6 +795,7 @@ mapAliases ({
parity = openethereum; # Added 2020-08-01
partition-manager = libsForQt5.partitionmanager; # Added 2024-01-08
pash = throw "'pash' has been removed: abandoned by upstream. Use 'powershell' instead"; # Added 2023-09-16
patchelfStable = patchelf; # Added 2024-01-25
pcsctools = pcsc-tools; # Added 2023-12-07
pdf2xml = throw "'pdf2xml' was removed: abandoned for years."; # Added 2023-10-22
peach = asouldocs; # Added 2022-08-28
Expand Down
9 changes: 1 addition & 8 deletions pkgs/top-level/all-packages.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19740,14 +19740,7 @@ with pkgs;

parse-cli-bin = callPackage ../development/tools/parse-cli-bin { };

patchelf = if with stdenv.buildPlatform; isAarch64 && isMusl then
patchelf_0_13
else
patchelfStable;
patchelf_0_13 = callPackage ../development/tools/misc/patchelf/0.13.nix {
patchelf = patchelfStable;
};
Comment on lines -19747 to -19749
Copy link
Member

@infinisil infinisil Jan 25, 2024

Choose a reason for hiding this comment

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

Attribute is being removed without a throwing alias

Copy link
Member Author

@alyssais alyssais Jan 25, 2024

Choose a reason for hiding this comment

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

Is that really mandatory? Isn't it pretty obvious what's happened if a versioned attribute you were using disappears? (To the extent it's likely that anybody was explicitly using this version introduced just to be the default on aarch64-musl in the first place…)

Copy link
Member

@infinisil infinisil Jan 25, 2024

Choose a reason for hiding this comment

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

While the API surface of Nixpkgs isn't explicitly declared somewhere yet, it's pretty much accepted that top-level attributes are effectively part of that. It's also not declared explicitly what stability guarantees Nixpkgs gives about such an API, but I think it's also fairly accepted that attributes should continue existing, that's why we have aliases.

Ideally I'd favor a deprecation message for one release, but in the absence of that we should at least have throwing aliases. Just removing attributes entirely is a bit too much.

And we should assume that any part of the API ends up getting used. Even if we don't hear from them, there likely is some user (or more) out there getting annoyed over attributes randomly disappearing. We can improve those users experience by being more diligent with aliases, I think we should go for that :)

Copy link
Member

@infinisil infinisil Jan 25, 2024

Choose a reason for hiding this comment

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

Don't bother with this if you don't want to (though I'd merge your PR if you did). But just for the future I think it would be good to have aliases for all removed attributes.

Copy link
Member Author

@alyssais alyssais Jan 25, 2024

Choose a reason for hiding this comment

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

And we should assume that any part of the API ends up getting used. Even if we don't hear from them, there likely is some user (or more) out there getting annoyed over attributes randomly disappearing.

We change little-used APIs all the time. There's no way it's possible to maintain our ability to iterate if we have to keep every reachable attribute working and unchanging for a two-cycle deprecation period.

Copy link
Member

@infinisil infinisil Jan 26, 2024

Choose a reason for hiding this comment

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

if we have to keep every reachable attribute working and unchanging for a two-cycle deprecation period

Yeah, but that's not the case for a throwing alias, which costs effectively nothing

patchelfStable = callPackage ../development/tools/misc/patchelf { };
patchelf = callPackage ../development/tools/misc/patchelf { };

patchelfUnstable = lowPrio (callPackage ../development/tools/misc/patchelf/unstable.nix { });

Expand Down