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

llvmPackages.bintools-unwrapped: add missing symlinks #211161

Merged
merged 2 commits into from
Jan 18, 2023

Conversation

alyssais
Copy link
Member

Description of changes

We were missing symlinks for some programs e.g. strings, which caused
e.g. pkgsLLVM.x264 to fail to build.

Here, I have filled in all the symlinks that LLVM would create if
built with the LLVM_INSTALL_BINUTILS_SYMLINKS option. Where an
existing symlink's target has changed, it's to avoid a double
indirection e.g. strip -> llvm-strip -> llvm-objcopy has because just
strip -> llvm-objcopy.

There's also the related problem that we are creating a as -> llvm-as
symlink, which doesn't make sense, but I've removed that in a
separate commit so that if it somehow breaks something it's easy to
revert just that change.

"llvm-as is an LLVM IR -> LLVM bitcode assembler not a system
assembler"1, and therefore should not be linked as "as".

The "as" symlink was removed in 46e5ea5 ("llvm*: remove symlinks
to llvm-diff, llvm-as and associated LLVM IR utilities."), but that
was partially reverted by b331c72 ("llvm: setup some symlinks for
compatibility with binutils"), which restored a bunch of symlinks that
were incorrectly removed, but also incorrectly restored "as". This
was pointed out2 at the time but apparently never fixed.

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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

We were missing symlinks for some programs e.g. strings, which caused
e.g. pkgsLLVM.x264 to fail to build.

Here, I have filled in all the symlinks that LLVM would create if
built with the LLVM_INSTALL_BINUTILS_SYMLINKS option.  Where an
existing symlink's target has changed, it's to avoid a double
indirection e.g. strip -> llvm-strip -> llvm-objcopy has because just
strip -> llvm-objcopy.

There's also the related problem that we are creating a as -> llvm-as
symlink, which doesn't make sense, but I'll remove that in a
subsequent commit so that if it somehow breaks something it's easy to
revert just that change.

Fixes: NixOS#210983
"llvm-as is an LLVM IR -> LLVM bitcode assembler not a system
assembler"[1], and therefore should not be linked as "as".

The "as" symlink was removed in 46e5ea5 ("llvm*: remove symlinks
to llvm-diff, llvm-as and associated LLVM IR utilities."), but that
was partially reverted by b331c72 ("llvm: setup some symlinks for
compatibility with binutils"), which restored a bunch of symlinks that
were incorrectly removed, but also incorrectly restored "as".  This
was pointed out[2] at the time but apparently never fixed.

[1]: NixOS#93523 (comment)
[2]: NixOS@b331c72#commitcomment-40981705
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.

What made you decide against LLVM_INSTALL_BINUTILS_SYMLINKS?

@winterqt
Copy link
Member

What made you decide against LLVM_INSTALL_BINUTILS_SYMLINKS?

I presume the LLVM rebuild required? Though I think that may be better long-term if that's the only reason. (We do symlink other things manually even before this, though? IDK.)

@alyssais
Copy link
Member Author

I decided to do it this way mostly because I noticed we were already doing it, and I didn't feel like there was a good reason to change. I didn't want to build with symlinks all the time, because I worried that could interfere with GNU stdenv builds, and I didn't want to add another useLLVM check to decide whether to enable it because that's on the way out.

@alyssais alyssais merged commit a5e1363 into NixOS:master Jan 18, 2023
@alyssais alyssais deleted the llvm-strings branch January 18, 2023 11:36
@rrbutani rrbutani mentioned this pull request Jan 25, 2023
92 tasks
rrbutani added a commit to rrbutani/nixpkgs that referenced this pull request Jan 27, 2023
github-actions bot pushed a commit that referenced this pull request Jan 28, 2023
@winterqt
Copy link
Member

Sorry, just getting back to this.

and I didn't want to add another useLLVM check to decide whether to enable it because that's on the way out.

What did you mean by this? I can't find any e.g. recent issues surrounding this.

xanderio pushed a commit to xanderio/nixpkgs that referenced this pull request Feb 13, 2023
@sternenseemann
Copy link
Member

@winterqt The relevant code is littered with comments like this one:

# Choose what linker we wish to use by default. Someday we might also
# choose the C compiler, runtime library, C++ standard library, etc. in
# this way, nice and orthogonally, and deprecate `useLLVM`. But due to
# the monolithic GCC build we cannot actually make those choices
# independently, so we are just doing `linker` and keeping `useLLVM` for
# now.

@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 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants