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.

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

lld: build with 2M stack size on musl #218657

Merged
merged 2 commits into from
Feb 28, 2023
Merged

Conversation

alyssais
Copy link
Member

Description of changes

Port of 6485a02 ("llvm 14 lld: build with 2M stack size to fix firefox lto").

LLVM 7 doesn't build for Musl for unrelated reasons, as it segfaults during a test. I gave up trying to fix it when I realised it doesn't do that if I enableDebugging.

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, 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.05 Release Notes (or backporting 22.11 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.

@alyssais alyssais added the 6.topic: musl Running or building packages with musl libc label Feb 27, 2023
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.

Commit message should read lld though, maybe indicate somehow that it is about all available versions.

Port of 6485a02 ("llvm 14 lld: build with 2M stack size to fix
firefox lto").
@alyssais
Copy link
Member Author

maybe indicate somehow that it is about all available versions.

I think we should get into the habit of being surprised when an LLVM change is not applied to all versions, not when it is.

Copy link
Contributor

@rrbutani rrbutani left a comment

Choose a reason for hiding this comment

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

Subjective (and something I should have raised on the original PR or in the LLVM 15 PR) so feel free to ignore:

I'd really appreciate a comment here explaining why this is necessary (perhaps with a link to this (and maybe this/this) and a short snippet saying that the pkgsMusl.firefox needs this -- or even just a link to #191372).

IMO it's a little hard to infer why this is needed from the surrounding context and while all the details are definitely still available in PR comments/commit messages, with our "copy the previous llvmPackages's files" flow it takes a bit of effort to map back to the original commit (though I guess having llvmPackages_git and keeping it up to date remedies this, somewhat).

Suggested-by: Rahul Butani <rrbutani@users.noreply.github.com>
@alyssais
Copy link
Member Author

I'd really appreciate a comment here explaining why this is necessary (perhaps with a link to this (and maybe this/this) and a short snippet saying that the pkgsMusl.firefox needs this -- or even just a link to #191372).

Done. I explained inline rather than linking though, because links rot.

@alyssais alyssais changed the title llvm: build with 2M stack size on musl lld: build with 2M stack size on musl Feb 28, 2023
@alyssais alyssais merged commit 1db7f30 into NixOS:staging Feb 28, 2023
@alyssais alyssais deleted the lld-stack-size branch February 28, 2023 10:56
@rrbutani rrbutani added the 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related label May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 6.topic: musl Running or building packages with musl libc 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants