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

gcc11: Update back to 11.3 on Darwin #196565

Merged
merged 2 commits into from
Nov 22, 2022
Merged

gcc11: Update back to 11.3 on Darwin #196565

merged 2 commits into from
Nov 22, 2022

Conversation

zhaofengli
Copy link
Member

@zhaofengli zhaofengli commented Oct 18, 2022 •
edited
Loading

Description of changes

Update back to GCC 11.3 for Darwin, with the patchset used by Homebrew. Removed libgcc dependency from graphite2 as it does not appear to be needed (tested on aarch64-darwin and x86_64-darwin).

Description of removed patch

#171792 downgraded gcc11 to 11.2 because it broke the standalone libgcc:

make: *** No rule to make target 'emutls_s.o', needed by 'libemutls_w.a'.  Stop.

Because we build the standalone libgcc with --disable-shared, libemutls_s.o is natually not built. I wrote a small patch to fix it.

Ref: #137877

Things done
  • Built on platform(s)
    • x86_64-linux (on master, for the libgcc change)
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin (on master)
  • 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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Oct 18, 2022
@zhaofengli zhaofengli requested review from vcunat and a team October 18, 2022 09:48
@zhaofengli zhaofengli mentioned this pull request Oct 18, 2022
Closed
@vcunat vcunat removed their request for review October 18, 2022 16:37
@vcunat
Copy link
Member

vcunat commented Oct 18, 2022

No objection from me, but I won't do a proper review or testing of this.

veprbl
veprbl approved these changes Oct 18, 2022
View reviewed changes
Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

LGTM

pkgs/development/compilers/gcc/11/default.nix Outdated
url = "https://github.com/Homebrew/formula-patches/raw/8184603db1db7da12d0fbfa47e1329ba0e21558e/gcc/gcc-11.3.0-arm.diff";
sha256 = "sha256-sEClJ7BzXJccLm+V6LF9Ioe4o/aV3iJ4akvOVoPG5pA=";
})
./libgcc-darwin-libemutls-no-shared.patch
Copy link
Member

Choose a reason for hiding this comment

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

If you've submitted this upstream, please include a link

@zhaofengli
Copy link
Member Author

Even though this is probably too late for 22.11, is it possible to get a jobset so we can test in the meantime?

@vcunat
Copy link
Member

vcunat commented Oct 18, 2022

Yes, but I believe it's inconvenient for the current commits. They're in a place where even stdenv binaries are missing, and I'm not speaking of possible regressions that haven't been discovered there yet. Better do that with this pair of commits based on recent master or at least staging-next branch (though there's still a large rustc regression on staging-next).

@vcunat
Copy link
Member

vcunat commented Oct 18, 2022 •
edited
Loading

And I'm personally convinced that this PR would fall under that 22.11 freeze. The gcc condition probably doesn't really apply on Darwin, and minor update shouldn't be a "breaking change".

@vcunat
Copy link
Member

vcunat commented Oct 19, 2022

Builds in progress at https://hydra.nixos.org/jobset/nixpkgs/pr-196565-darwin-gcc11.3

@vcunat
Copy link
Member

vcunat commented Oct 19, 2022

Though I'm surprised that the testing branch is based on a commit from August (0f5230c).

@zhaofengli
Copy link
Member Author

Though I'm surprised that the testing branch is based on a commit from August (0f5230c).

Oops, the local master ref in my nixpkgs checkout on my Mac was severely outdated. Rebased.

@vcunat
Copy link
Member

vcunat commented Oct 19, 2022

So, diffs in failures can be observed (after rebuilds progress) like

@zhaofengli zhaofengli mentioned this pull request Oct 19, 2022
Closed
toonn
toonn approved these changes Oct 19, 2022
View reviewed changes
Copy link
Contributor

@toonn toonn left a comment

Choose a reason for hiding this comment

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

WFM on x86_64-darwin.

@zhaofengli
Copy link
Member Author

Let's put this on hold while we wait for clarifications from @Ericson2314 regarding the libgcc package (iains/gcc-darwin-arm64#99).

graphite2, the only package buildable on Darwin requiring this, doesn't seem to require it at all now (cc @sternenseemann, author of #123420).

@zhaofengli zhaofengli marked this pull request as draft October 19, 2022 19:19
@zhaofengli
graphite2: Remove libgcc
2e38eee 
It doesn't seem to be needed.
@zhaofengli
Copy link
Member Author

In the interest of pushing this forward, let's remove the libgcc patch for now. I removed libgcc dependency from graphite2 as it does not appear to be needed (tested on aarch64-darwin and x86_64-darwin).

@zhaofengli zhaofengli marked this pull request as ready for review October 24, 2022 06:16
@ofborg ofborg bot requested review from vcunat and 7c6f434c October 24, 2022 06:23
@ofborg ofborg bot added 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux and removed 10.rebuild-linux: 1-10 labels Oct 24, 2022
@zhaofengli
Copy link
Member Author

I changed the patch URL as suggested and tested on darwin-gcc-11-3-master. Now we need someone to trigger the eval on Hydra again.

@zhaofengli
Copy link
Member Author

zhaofengli commented Nov 5, 2022 •
edited
Loading

It's been a week, and here are the results of the new eval:

There are a lot of timeouts with empty logs which I'm not sure how to fix.

@vcunat
Copy link
Member

vcunat commented Nov 5, 2022

Those were issues in the builder machines; I can restart the builds.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/685

SuperSandro2000
SuperSandro2000 reviewed Nov 20, 2022
View reviewed changes
pkgs/development/compilers/gcc/11/default.nix
# On x86_64-darwin, building libgcc suffers from some different issues with 11.3.0.
version = if stdenv.isDarwin then
"${majorVersion}.2.0" else "${majorVersion}.3.0";
version = "${majorVersion}.3.0";
Copy link
Member

Choose a reason for hiding this comment

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

Instead of manually setting majorVersion we should use lib.versions.major version

Copy link
Member

Choose a reason for hiding this comment

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

Could be done, but on all versions consistently... and it doesn't seem very related to this PR.

@vcunat vcunat merged commit befa1f2 into NixOS:staging Nov 22, 2022
@zhaofengli zhaofengli deleted the darwin-gcc-11-3 branch November 22, 2022 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@toonn toonn toonn approved these changes

@veprbl veprbl veprbl approved these changes

@bobrik bobrik bobrik approved these changes

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

@Synthetica9 Synthetica9 Awaiting requested review from Synthetica9

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@vcunat vcunat Awaiting requested review from vcunat

@7c6f434c 7c6f434c Awaiting requested review from 7c6f434c

Assignees
No one assigned
Labels
6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@zhaofengli @vcunat @nixos-discourse @bobrik @veprbl @toonn @SuperSandro2000