Skip to content

llvmPackages.bintools-unwrapped: add missing symlinks#211161

Merged
alyssais merged 2 commits intoNixOS:masterfrom
alyssais:llvm-strings
Jan 18, 2023
Merged

llvmPackages.bintools-unwrapped: add missing symlinks#211161
alyssais merged 2 commits intoNixOS:masterfrom
alyssais:llvm-strings

Conversation

@alyssais
Copy link
Member

@alyssais alyssais commented Jan 17, 2023

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.

@alyssais
llvmPackages.bintools-unwrapped: add missing symlinks
363cca9 
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
@alyssais
llvmPackages.bintools: remove as -> llvm-as symlink
41494b4 
"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
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Jan 17, 2023
@alyssais alyssais changed the base branch from staging to master January 17, 2023 10:45
sternenseemann
sternenseemann approved these changes Jan 18, 2023
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.

What made you decide against LLVM_INSTALL_BINUTILS_SYMLINKS?

@winterqt
Copy link
Member

winterqt commented Jan 18, 2023

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

alyssais commented Jan 18, 2023

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
Merged
92 tasks
rrbutani added a commit to rrbutani/nixpkgs that referenced this pull request Jan 27, 2023
@rrbutani
llvmPackages_15: apply some patches from llvmPackages_14, part 2
9bd9267 
See NixOS#194634 (comment) for
details.

PRs:
  - NixOS#211401
  - NixOS#211161
  - NixOS#206742
  - NixOS#211687
github-actions bot pushed a commit that referenced this pull request Jan 28, 2023
@rrbutani @github-actions
llvmPackages_15: apply some patches from llvmPackages_14, part 2
2bc885c 
See #194634 (comment) for
details.

PRs:
  - #211401
  - #211161
  - #206742
  - #211687
(cherry picked from commit 9bd9267)
K900 pushed a commit that referenced this pull request Jan 28, 2023
@rrbutani @K900
llvmPackages_15: apply some patches from llvmPackages_14, part 2
735a99c 
See #194634 (comment) for
details.

PRs:
  - #211401
  - #211161
  - #206742
  - #211687
@winterqt
Copy link
Member

winterqt commented Jan 28, 2023

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
@rrbutani @xanderio
llvmPackages_15: apply some patches from llvmPackages_14, part 2
335515c 
See NixOS#194634 (comment) for
details.

PRs:
  - NixOS#211401
  - NixOS#211161
  - NixOS#206742
  - NixOS#211687
@sternenseemann
Copy link
Member

sternenseemann commented Feb 19, 2023

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

nixpkgs/lib/systems/default.nix

Lines 56 to 61 in b467aaa

# 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

Reviewers

@sternenseemann sternenseemann sternenseemann approved these changes

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@winterqt winterqt Awaiting requested review from winterqt

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

Assignees

No one assigned

Labels

6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@alyssais @winterqt @sternenseemann @rrbutani