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.

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

Jump to bottom

[staging] cdparanoia: cleanup, fix build on powerpc #106413

Merged
merged 1 commit into from Jun 16, 2021
Merged

[staging] cdparanoia: cleanup, fix build on powerpc #106413

merged 1 commit into from Jun 16, 2021

Conversation

r-burns
Copy link
Contributor

@r-burns r-burns commented Dec 9, 2020

The configure helpers are old and already need to be updated on aarch64,
so let's just do this on all non-x86 platforms. This will probably fix
other architectures that weren't well-supported at the time.

Motivation for this change

Fixes build on ppc64le (and possibly others)

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@JohnAZoidberg
Copy link
Member

Does it build on powerpc like this?
I also see

  nativeBuildInputs = lib.optional stdenv.isAarch64 autoreconfHook;

should this also be nonx86 then?

@r-burns
Copy link
Contributor Author

r-burns commented Feb 9, 2021

Huh, I didn't notice that bit, and it is indeed not needed on powerpc, as the derivation builds for me as is. Interestingly, I'm able to remove the autoreconfHook on aarch64 and it still builds there as well. I think updating the configure.{sub,guess} scripts is sufficient, because they are only needed at configure time.
It looks like @dezgeg added this in c863aa3 - maybe there's something I'm missing, or it's no longer needed. Judging by the commit message, he may have just been fed up after wrestling with the build ;)

@JohnAZoidberg
Copy link
Member

Interestingly, I'm able to remove the autoreconfHook on aarch64 and it still builds there as well.

Cook, thanks for checking! Let's remove it then.

@r-burns
Copy link
Contributor Author

r-burns commented Feb 9, 2021

Ok, done! And just to be sure, I also checked x86 -> aarch64, x86 -> powerpc, and powerpc -> x86 cross.

@r-burns
cdparanoia: fix build on powerpc
9f88425 
The configure helpers are old and already need to be updated on aarch64,
so let's just do this on all non-x86 platforms. This will probably fix
other architectures that weren't well-supported at the time.

The autoreconfHook does not appear to be needed on aarch64 or powerpc.
@r-burns r-burns marked this pull request as draft February 9, 2021 20:40
@r-burns
Copy link
Contributor Author

r-burns commented Feb 9, 2021 •
edited

Head commit was working but the merge commit was failing (edit: not so sure about this, might have just gotten my git branches confused) on borg aarch64 because the configure snippet was being appended as args to unset. This is now a mass rebuild, so I'll target staging (after the next automatic master -> staging merge, so I don't cause a mass notification).

@r-burns r-burns changed the base branch from master to staging February 10, 2021 01:25
@r-burns r-burns marked this pull request as ready for review February 10, 2021 01:25
@r-burns r-burns changed the title cdparanoia: fix build on powerpc [staging] cdparanoia: fix build on powerpc Feb 10, 2021
@r-burns r-burns changed the title [staging] cdparanoia: fix build on powerpc [staging] cdparanoia: cleanup, fix build on powerpc Feb 10, 2021
@JohnAZoidberg
Copy link
Member

Ok, done! And just to be sure, I also checked x86 -> aarch64, x86 -> powerpc, and powerpc -> x86 cross.

Amazing, thanks!

Head commit was working but the merge commit was failing on borg aarch64 because the configure snippet was being appended as args to unset. This is now a mass rebuild, so I'll target staging (after the next automatic master -> staging merge, so I don't cause a mass notification).

Hmm, I don't quite understand. Why does it cause a mass rebuild now, but not before?

@r-burns
Copy link
Contributor Author

r-burns commented Feb 12, 2021

Haha I was super confused for a minute but now I see what the issue was. See the semicolon at the end of this line?
https://github.com/NixOS/nixpkgs/blob/ede9879e64b9cf477c5f1677601421d2d4dc1bf2/pkgs/applications/audio/cdparanoia/default.nix#L32
That actually goes into the shell, and is what keeps the first cp command from being appended as an arg to unset. Somewhere along the line I lost that semicolon and ofborg started complaining.

I kind of want to get rid of this weird trick and just put the unset command in its own multiline string, although doing so does cause a mass rebuild. Up to you I guess.

JohnAZoidberg
JohnAZoidberg approved these changes Feb 12, 2021
View reviewed changes
Copy link
Member

@JohnAZoidberg JohnAZoidberg left a comment

Choose a reason for hiding this comment

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

Ohhh, I see what's happening here. cdparanoia is a transitive dependency of all of those packages.
Before your change the nix expression would evaluate to the same derivation but now it doesn't anymore.

Yeah that semicolon is definitely not obvious, I think it's good to change that.

@r-burns
Copy link
Contributor Author

r-burns commented Mar 29, 2021

@JohnAZoidberg I see you are a committer, do you think this is ready for merge? Or shall I solicit for further review?

@zhaofengli
Copy link
Member

Can confirm that this fixes the build on riscv64 as well. Let's get this merged.

@zhaofengli zhaofengli mentioned this pull request Jun 11, 2021
Closed
@mweinelt mweinelt merged commit ee31040 into NixOS:staging Jun 16, 2021
@r-burns r-burns deleted the cdparanoia branch June 16, 2021 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JohnAZoidberg JohnAZoidberg JohnAZoidberg approved these changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 11-100 10.rebuild-linux: 501+ 10.rebuild-linux: 1001-2500
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@r-burns @JohnAZoidberg @zhaofengli @mweinelt @SuperSandro2000