Skip to content

Comments

Linux bump default LLVM to 11#142593

Merged
mweinelt merged 8 commits intoNixOS:stagingfrom
toonn:linux-llvm-bump
Feb 1, 2022
Merged

Linux bump default LLVM to 11#142593
mweinelt merged 8 commits intoNixOS:stagingfrom
toonn:linux-llvm-bump

Conversation

@toonn
Copy link
Contributor

@toonn toonn commented Oct 22, 2021

Motivation for this change

#126411 bumps the default version of LLVM for x86_64 Darwin, which is tied to the stdenv. This brings it in line with aarch64-darwin but takes it out of sync with the default LLVM for Linux. This is not ideal because fixes for one LLVM version could introduce failures for another version, which would require introducing branching on the LLVM version to resolve.

This PR builds on #126411 and is currently mostly to get a Hydra evaluation for Linux to see how much work bumping would take.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages 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/)
  • 21.11 Release Notes (or backporting 21.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.

@github-actions github-actions bot added 6.topic: printing Drivers, CUPS & Co. 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: qt/kde Object-oriented framework for GUI creation 6.topic: stdenv Standard environment labels Oct 22, 2021
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Oct 22, 2021
@alyssais
Copy link
Member

alyssais commented Oct 22, 2021

As we're working on this, I'd like to suggest that we send fixes straight to master/staging as their own PRs where doing so wouldn't break builds on LLVM 7. That should keep PRs small, and make things much easier to review.

@vcunat
Copy link
Member

vcunat commented Oct 22, 2021

https://hydra.nixos.org/eval/1715423

@toonn
Copy link
Contributor Author

toonn commented Oct 23, 2021

Looks like there's about 6-7k newly failing builds, this is the comparison I'm using. Looks like the cups patch I already got rid of is the main issue. I will rebase this on the Darwin bump as soon as 2 builds I'm running to check the changes finish.

@nixos-discourse
Copy link

nixos-discourse commented Nov 10, 2021

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

https://discourse.nixos.org/t/nix-macos-monthly/12330/12

@github-actions github-actions bot removed 6.topic: printing Drivers, CUPS & Co. 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: stdenv Standard environment labels Dec 21, 2021
@ofborg ofborg bot added 8.has: clean-up This PR removes packages or removes other cruft and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Dec 21, 2021
@ofborg ofborg bot requested a review from matthewbauer December 21, 2021 21:17
@ofborg ofborg bot added 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches. labels Dec 21, 2021
@toonn
Copy link
Contributor Author

toonn commented Dec 21, 2021

Just rebased on staging-next since all the release-related stuff has cooled down and the bump on Darwin seems to have gone into staging-next OK. Here's a new comparison.

@veprbl veprbl mentioned this pull request Dec 25, 2021
Merged
13 tasks
@veprbl veprbl changed the base branch from staging-next to staging December 25, 2021 21:53
@sternenseemann sternenseemann marked this pull request as ready for review December 26, 2021 10:11
@sternenseemann
Copy link
Member

sternenseemann commented Dec 26, 2021

Seems like 400 potential new regressions compared to current master? https://hydra.nixos.org/eval/1732639?compare=trunk#tabs-now-fail

@toonn
Copy link
Contributor Author

toonn commented Dec 27, 2021

@sternenseemann, yep, what number of failures would be considered good enough? Should I rebase this on master or staging-next again to get a more recent comparison? The staging-next evaluation this one is based on has even more failures when compared to trunk.

@toonn
Copy link
Contributor Author

toonn commented Jan 11, 2022

So, in this staging-next comparison, from ~ two weeks ago, the main cause of failures seemed to be ispc. This has since been worked around in 981170a so I rebased the branch again.

How do we move this forward? Last comparison we had 23 newly introduced failures as compared to staging-next. Is the goal/requirement to introduce 0 new failures?

@vcunat
Copy link
Member

vcunat commented Jan 11, 2022

No, even staging-next -> master merges don't target zero regressions, though it's nice when they get very low. If it's like 23 and nothing important stands out, I think it's OK to merge to staging and a further round of reduction can be done during staging-next builds.

toonn added 3 commits January 17, 2022 13:29
@toonn
llvmPackages: Update Linux default to 11
bf09623 
Update the default LLVM version on Linux to stay synchronized with
Darwin, where LLVM is tied to the stdenv.
@toonn
bear: Darwin stdenv is now LLVM 11 by default
a0283ba
@toonn
c3c: llvmPackages defaults to LLVM 11 on the main platforms
54ac4a7
@toonn
Copy link
Contributor Author

toonn commented Jan 17, 2022

Ok, so this is the comparison for the previous evaluation.
The only thing that stands out to me is Blender. Which is still failing because of ispc, so I looked for the most recent evaluation where ispc succeeds and started a new evaluation to make sure.

This confirmed that I did indeed break ispc. Turns out when removing the LLVM 11 specific arguments I removed stdenv = llvmPackages_11.stdenv from the arguments for ispc. I was probably thinking about the darwin stdenv too much, which does use LLVM. The next evaluation should have even fewer failures 🤞.

@toonn
Copy link
Contributor Author

toonn commented Jan 24, 2022

The ispc failure (and Blender) is fixed now. Zcash was also using an LLVM stdenv but seems to work fine with a GNU stdenv so I'm not sure that's still necessary? @vcunat was so kind as to check that webkitgtk_4_1 does indeed build, I'm not sure why it failed on Hydra (OOM?).

I believe this means there's no more serious failures so this is ready for review/testing.

@toonn
Copy link
Contributor Author

toonn commented Jan 25, 2022

@alyssais, @sternenseemann, @vcunat, I believe this is ready for review.

sternenseemann
sternenseemann approved these changes Jan 27, 2022
View reviewed changes
Copy link
Member

@sternenseemann sternenseemann left a comment

Choose a reason for hiding this comment

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

Haven't checked if relying on the default version in all cases is wise (depends on the package), but probably doesn't matter in the grand scheme of things (every default LLVM update will need careful checking for regressions).

@toonn
Copy link
Contributor Author

toonn commented Jan 29, 2022

My motivation for using the default is an assumption that most things that explicitly specify LLVM 11 probably do so because the default isn't recent enough.

jonringer
jonringer approved these changes Feb 1, 2022
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

According to the hydra eval, most failures are related to the recent python310 bump

# nix-review-tool output
x86_64-linux python3.10-unittest2-1.1.0 | 91
x86_64-linux python3.10-debugpy-1.5.1 | 58
x86_64-linux python3.10-imagecodecs-lite-2019.12.3 | 29
x86_64-linux python3.10-eventlet-0.33.0 | 27
x86_64-linux sybil-2.0.1 | 19
x86_64-linux python3.10-pytest-4.6.11 | 18
x86_64-linux python3.10-semver-2.13.0 | 17
x86_64-linux python3.9-pygame-2.1.0 | 14
x86_64-linux python3.10-tensorflow-tensorboard-2.6.0 | 12
x86_64-linux webkitgtk-2.34.3 | 11
x86_64-linux python3.10-mpi4py-3.0.3 | 9
x86_64-linux python3.10-commoncode-30.0.0 | 9
x86_64-linux python3.10-sanic-21.9.3 | 9
x86_64-linux python3.10-cssutils-2.3.0 | 8
x86_64-linux python3.10-unicodedata2-13.0.0-2 | 7
x86_64-linux python3.10-mocket-3.10.3 | 7

mweinelt
mweinelt approved these changes Feb 1, 2022
View reviewed changes
Copy link
Member

@mweinelt mweinelt left a comment

Choose a reason for hiding this comment

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

Current comparison against trunk looks very good indeed.

@mweinelt mweinelt merged commit 184d6ba into NixOS:staging Feb 1, 2022
@toonn
Copy link
Contributor Author

toonn commented Feb 2, 2022

Thanks, @jonringer and @mweinelt!

@Atemu Atemu mentioned this pull request Mar 30, 2022
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mweinelt mweinelt mweinelt approved these changes

@sternenseemann sternenseemann sternenseemann approved these changes

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

@ttuegel ttuegel Awaiting requested review from ttuegel

+1 more reviewer

@jonringer jonringer jonringer approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: qt/kde Object-oriented framework for GUI creation 8.has: clean-up This PR removes packages or removes other cruft 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@toonn @alyssais @vcunat @nixos-discourse @sternenseemann @mweinelt @jonringer