Skip to content

gcc: use standard builder instead of custom builder.sh#237856

Merged
Artturin merged 9 commits intostagingfrom
unknown repository
Jul 21, 2023
Merged

gcc: use standard builder instead of custom builder.sh#237856
Artturin merged 9 commits intostagingfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Jun 15, 2023 •
edited by ghost
Loading

Please review the commits in this PR separately, ignoring whitespace.

If you read them that way, most of them are trivial. You can skip the first six commits, which are from this PR (merged to master but has not yet reached staging):

Description of changes

This PR causes our gcc expressions (all 10 of them) to use the standard builder instead of their own custom builder.sh.

This PR deliberately does the minimum amount possible to achieve that, because it is likely to merge-conflict with almost any other PR which touches any version of GCC. Most of the commits in this PR are simple, repeatable operations (like search and replace) to try to make it easier to rebase them past any conflicts.

Most of the benefits of this PR will be realized in future PRs (see the last few commits of #238069)

Future Benefits
  • Right now changing even a single bit in gcc/builder.sh causes a mass rebuild of every package on every platform. This is crazy.
    • I would like to do a significant amount of work on our cross-compilation scheme, and almost none of that work should trigger mass rebuilds or have to go through staging.
  • Overriding build phases is painful with builder.sh; see the gymnastics with "extra fixup phases" I had to add to libgcc.nix. Those will go away.
  • We're currently passing a ton of variables into the builder environment, many of which could be accessed directly by Nix conditionals.

And, ultimately the big long-term benefit:

  • Stop maintaining 10 copies of everything, and risking subtle hard-to-notice bugs if one copy gets out of sync with the others. Our gcc expression should look like our boost and nix expressions, using lib.versions when it is necessary to set minimum versions for things like patch applicability or configure flags.
Things done
  • Built on platform(s)
    • x86_64-linux gcc48,gcc49,gcc6,gcc7,gcc8,gcc9,gcc10,gcc11,gcc12,gcc13
    • aarch64-linux (cross from x86_64-linux): in progress
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.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.

@ghost ghost changed the title gcc: use common/builder.nix gcc: use standard builder instead of custom builder.sh Jun 15, 2023
@ghost ghost mentioned this pull request Jun 16, 2023
Closed
12 tasks
@ghost ghost marked this pull request as ready for review June 16, 2023 09:36
@ghost ghost requested a review from matthewbauer as a code owner June 16, 2023 09:36
@ofborg ofborg bot added the 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. label Jun 16, 2023
@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. labels Jun 16, 2023
@ghost ghost requested a review from trofi June 16, 2023 09:45
trofi
trofi previously approved these changes Jun 16, 2023
View reviewed changes
Copy link
Contributor

@trofi trofi left a comment

Choose a reason for hiding this comment

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

Looks good.

diffoscope does not show any material changes before/after at least for gcc.cc.

@trofi
Copy link
Contributor

trofi commented Jun 16, 2023

Having said that wineWowPackages.waylandFull.stdenv.cc.cc (a dependency of wineWowPackages.waylandFull) somehoow fails to build for me now. Has something to do with library search path (might also be my local patches):

/nix/store/163svia93c95033jqv72yin4vwm4s8v8-binutils-patchelfed-ld-2.40/bin/ld: skipping incompatible /nix/store/8gf90snmvzp1sg5h68lrn1qiw5vchnfg-glibc-2.37-8/lib/libc.so when searching for -lc

@figsoda figsoda added the 12.approvals: 1 This PR was reviewed and approved by one person. label Jun 18, 2023
@ghost ghost marked this pull request as draft June 28, 2023 05:45
@ghost

This comment was marked as resolved.

Sign in to view
@github-actions github-actions bot added 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: nim Nim programing language labels Jul 1, 2023
@trofi

This comment was marked as resolved.

Sign in to view
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Jul 1, 2023
@github-actions github-actions bot removed 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: nim Nim programing language labels Jul 2, 2023
@ghost

This comment was marked as resolved.

Sign in to view
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 2, 2023
Adam Joseph added 3 commits July 7, 2023 05:55
gcc: copy gcc/builder.sh to gcc/common/builder.nix
caf22a3 
This commit is left unsquashed to make review of the subsequent
commits easier.
gcc: make common/builder.nix into valid Nix syntax
72284b0 
This commit performs two search-and-replace operations:

- replace all `${` with `''${`
- replace `''` with `""` in shell scripts

This commit is left unsquashed to make review of the subsequent
commits easier.
gcc: add args to common/builder.nix, split up script into phases
1157e1d 
This commit adds arguments to `builder.nix`, making it a callable
function, and splits up the single massive shell script into
separate attributes for each phase.  It also drops the first two
lines and the last line because these are part of the default
builder.

All script lines which were not part of a phase function have been
moved into `preUnpack` since this is the first phase that runs.
Subsequent commits will move parts of this to more sensible
locations.
@github-actions github-actions bot removed the 8.has: documentation This PR adds or changes documentation label Jul 12, 2023
@ghost

This comment was marked as outdated.

Sign in to view
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 12, 2023
@ofborg ofborg bot requested review from SuperSandro2000, adevress and prusnak July 13, 2023 01:16
@ghost

This comment was marked as outdated.

Sign in to view
@eclairevoyant eclairevoyant mentioned this pull request Jul 13, 2023
Merged
12 tasks
@ghost
Copy link
Author

ghost commented Jul 13, 2023

Okay, I found the problem with gcc_multi, so I think this PR is ready.

Since last review I have:

  1. Rebased (no other changes)
  2. Pushed c76e34af5cbc6364d11e64c053a9b751d901f51d which simply replaces callPackage with callFile
  3. Pushed e02be1883fb2f1eacde8b193400c2168fce20737 which fixes the problem with wineWowPackages.waylandFull.stdenv.cc.cc.

So I think e02be1883fb2f1eacde8b193400c2168fce20737 is the only commit that needs additional review.

$ git rev-parse HEAD
e02be1883fb2f1eacde8b193400c2168fce20737
$ nix-instantiate . -A wineWowPackages.waylandFull.stdenv.cc.cc
warning: you did not specify '--add-root'; the result might be removed by the garbage collector
/nix/store/3i9dfhm3kwn269gblv0clqh05lypflm6-gcc-12.3.0.drv
$ nix build -f . wineWowPackages.waylandFull.stdenv.cc.cc && echo ok
ok

@ofborg build wineWowPackages.waylandFull.stdenv.cc.cc

@ghost ghost marked this pull request as ready for review July 13, 2023 02:42
@ghost ghost requested a review from alyssais as a code owner July 13, 2023 02:42
@ghost ghost requested a review from trofi July 13, 2023 02:42
@ghost
Copy link
Author

ghost commented Jul 13, 2023 •
edited by ghost
Loading

Building:

@ghost ghost mentioned this pull request Jul 13, 2023
Closed
@ghost
Copy link
Author

ghost commented Jul 13, 2023

wineWowPackages.waylandFull.stdenv.cc.cc on x86_64-linux — Success

@ghost ghost mentioned this pull request Jul 13, 2023
Closed
@trofi
Copy link
Contributor

trofi commented Jul 13, 2023

Looks like #241980 was lost (and maybe more?)

@trofi trofi removed their request for review July 13, 2023 05:14
@trofi trofi dismissed their stale review July 13, 2023 05:19

Large rebase, needs re-review.

@ghost
Copy link
Author

ghost commented Jul 13, 2023

Looks like #241980 was lost

Fixed, thanks for noticing.

@ghost ghost requested a review from trofi July 13, 2023 05:29
@trofi trofi removed their request for review July 13, 2023 05:34
@ghost
Copy link
Author

ghost commented Jul 13, 2023

@ofborg build perlPackages.XMLLibXML

@ghost
Copy link
Author

ghost commented Jul 20, 2023

Rebased to fix merge conflict.

@github-actions github-actions bot removed the 6.topic: lib The Nixpkgs function library label Jul 20, 2023
Artturin
Artturin approved these changes Jul 21, 2023
View reviewed changes
Copy link
Member

@Artturin Artturin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Let's see if anything breaks

Built pkgsCross.aarch64-multiplatform.bash

@Artturin Artturin merged commit 4f133e5 into NixOS:staging Jul 21, 2023
@ghost ghost deleted the pr/gcc/no-more-custom-builder branch July 21, 2023 09:01
Artturin
Artturin reviewed Jul 21, 2023
View reviewed changes
pkgs/development/compilers/gcc/common/builder.nix
Comment on lines +122 to +124
# It seems there is a bug in GCC 5
#"CFLAGS=$EXTRA_FLAGS $EXTRA_LDFLAGS"
#"CXXFLAGS=$EXTRA_FLAGS $EXTRA_LDFLAGS"
Copy link
Member

@Artturin Artturin Jul 21, 2023

Choose a reason for hiding this comment

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

This has been commented since it's introduction in 2017 d4595b3#diff-7e60bf21d59f0d31c6d1a2c867692b9bcbad6dc8d8611e06e2e7bb91a9e07877R116-R118

@Artturin Artturin mentioned this pull request Jan 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@Synthetica9 Synthetica9 Awaiting requested review from Synthetica9

@vcunat vcunat Awaiting requested review from vcunat

@veprbl veprbl Awaiting requested review from veprbl

@adevress adevress Awaiting requested review from adevress

@prusnak prusnak Awaiting requested review from prusnak

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

@alyssais alyssais Awaiting requested review from alyssais

Assignees

No one assigned

Labels

8.has: package (new) This PR adds a new package 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. 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@trofi @Artturin @figsoda