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

pkgsLLVM.x264: fix build #210983

Closed
wants to merge 1 commit into from
Closed

pkgsLLVM.x264: fix build #210983

wants to merge 1 commit into from

Conversation

alyssais
Copy link
Member

Description of changes
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 alyssais added the 6.topic: portability General portability concerns, not specific to cross-compilation or a specific platform label Jan 15, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jan 15, 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.

I guess this is o.k., but a better way to fix this, I feel like would be to make bintools wrapper link ${prefix}llvm-strings -> ${prefix}strings, given that it does link it in the GNU binutils case…

@@ -34,6 +34,10 @@ stdenv.mkDerivation rec {

outputs = [ "out" "lib" "dev" ];

STRINGS = if stdenv.hostPlatform.useLLVM or false
Copy link
Member

Choose a reason for hiding this comment

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

Hm, smells like we would need a stdenv.bintools.isLLVM or false of some kind…

Copy link
Member

@winterqt winterqt Jan 15, 2023

Choose a reason for hiding this comment

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

I guess LLVM bintools and GCC could be mixed, but would anyone actually do that? You're probably right, though, it's something that should be done for posterity if nothing else.

alyssais added a commit to alyssais/nixpkgs that referenced this pull request Jan 17, 2023
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 alyssais closed this in 3d1e039 Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: portability General portability concerns, not specific to cross-compilation or a specific platform 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants