llvm: init 5.0 #29541

dtzWill · 2017-09-18T20:25:36Z

Motivation for this change

New stable LLVM!

This does not change the default version anywhere, just makes the new version available.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

This should (and will be) squashed, postponing that until testing sorts out things like Darwin fixes that will require re-squashing anyway.

andrewrk · 2017-09-18T21:41:36Z

This does not change the default version anywhere, just makes the new version available.

If the default version was 4 before, it probably makes sense to change it to 5. Not really any backwards-incompatible things happened, except with internal C++ API stuff.

LnL7 · 2017-09-18T21:49:25Z

The darwin stdenv uses clang we should do some testing with a clang-5 stdenv before switching the default. Also I'm not sure if it's ideal to switch the default right after a new release since it potentially makes backporting more difficult.

Mic92 · 2017-09-19T08:29:19Z

pkgs/development/compilers/llvm/5/llvm.nix

+  '';
+
+  preCheck = ''
+    export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:$PWD/lib


Doing this in preBuild was not possible?

I'm not sure, would that be preferable? (why?)

(this doesn't make it right, but FWIW this is what's done for LLVM 4 currently so probably applies there as well)

Because there is a symlink to $out/lib added and removed.

Mic92 · 2017-09-19T08:31:00Z

pkgs/development/compilers/llvm/5/llvm-outputs.patch

+@@ -333,6 +333,21 @@ int main(int argc, char **argv) {
+     ActiveIncludeOption = "-I" + ActiveIncludeDir;
+   }
+


Btw is the output of llvm-config sane in this release? (Shows shared library correctly etc).

I gc'd my local build of this, whoops, so after that finishes I'll poke at it and check it looks reasonable when compared against the output given by LLVM 4's llvm-config.

If there's anything in particular you'd like to confirm works, LMK.

This pointed to static libraries in older unpatched versions:

$ llvm-config --libfiles /nix/store/7sa6x0qzqxnxlf9jy2vih66yzy12sja4-llvm-4.0.1-lib/lib/libLLVM-4.0.so

dtzWill · 2017-09-19T14:13:28Z

I agree that this should be mostly a smooth transition, but I'd still slightly prefer changing the default as a separate PR.

One reason is that I'm hoping to get this into 17.09 😇, and I expect that will be much less likely if the version is changed as well.. since the switch will likely be a separate commit this is mostly a matter of timing, since vetting the switch on Darwin understandably requires a fair amount of testing.

Mostly I'm just thinking that fallout from switching might be its own set of problems separate from the proper packaging of LLVM 5 and so deserves its own PR as logically distinct.

On the other hand, switching the default is a good way to test that things work reasonably-ish,
and without that these packages have no in-tree users (other than the stdenv tests I suppose hehe).

Anyway, lots of words to say: shrug! My opinion on this is a bit fickle, LMK if folks have preferences on the matter :).

dtzWill · 2017-09-19T14:19:21Z

If the default version was 4 before, it probably makes sense to change it to 5. Not really any backwards-incompatible things happened, except with internal C++ API stuff.

I think you're saying changing the default for users of clang/libc++ is likely not problematic? If so then I agree with your reasoning.

Users of the C++ API however are likely to break, although AFAIK the changes there are somewhat minor conceptually but still require attention.

IIRC the same version is used for both of these use cases so changing it impacts both types of users, although of course this could be routed around should that be useful.

dtzWill · 2017-09-19T14:21:10Z

The darwin stdenv uses clang we should do some testing with a clang-5 stdenv before switching the default. Also I'm not sure if it's ideal to switch the default right after a new release since it potentially makes backporting more difficult.

So what you're saying is we should switch the default /before/ the release, right?! 😁

(or backport the change once it's made)

andrewrk · 2017-09-19T14:30:58Z

Users of the C++ API however are likely to break, although AFAIK the changes there are somewhat minor conceptually but still require attention.

Users of the C++ API (myself being one of them) should lock in to a specific major version.

That said, I agree with your synopsis above about getting into 17.09.

LnL7 · 2017-09-19T17:18:43Z

Looks like something in the llvm build is trying to codesign on darwin.

x_dynamic.dir/lsan_thread.cc.o
[ 81%] Linking CXX shared library ../../../../lib/clang/5.0.0/lib/darwin/libclang_rt.lsan_osx_dynamic.dylib
ld: warning: linking against a dylib which is not safe for use in application extensions: /nix/store/550dnbd6xwdgv3nss7vr8zk8mpvhcx7d-libc++-4.0.1/lib/libc++.dylib
ld: warning: linking against a dylib which is not safe for use in application extensions: /nix/store/l8y6gvs5mnx0mb6dm4xbndvjhal5f0cp-libc++abi-5.0.0/lib/libc++abi.dylib
ld: warning: linking against a dylib which is not safe for use in application extensions: /nix/store/7ghdanyis355alwjrcrvlsfi7akc386q-Libsystem-osx-10.11.6/lib/libSystem.dylib
ld: warning: linking against a dylib which is not safe for use in application extensions: /nix/store/550dnbd6xwdgv3nss7vr8zk8mpvhcx7d-libc++-4.0.1/lib/libc++.dylib
ld: warning: linking against a dylib which is not safe for use in application extensions: /nix/store/l8y6gvs5mnx0mb6dm4xbndvjhal5f0cp-libc++abi-5.0.0/lib/libc++abi.dylib
ld: warning: linking against a dylib which is not safe for use in application extensions: /nix/store/7ghdanyis355alwjrcrvlsfi7akc386q-Libsystem-osx-10.11.6/lib/libSystem.dylib
/nix/store/hi7isb5axyx1h33nb5xyp0hwlcw7bp3k-bash-4.4-p12/bin/bash: codesign: command not found
make[2]: *** [projects/compiler-rt/lib/lsan/CMakeFiles/clang_rt.lsan_osx_dynamic.dir/build.make:384: lib/clang/5.0.0/lib/darwin/libclang_rt.lsan_osx_dynamic.dylib] Error 127
make[2]: *** Deleting file 'lib/clang/5.0.0/lib/darwin/libclang_rt.lsan_osx_dynamic.dylib'
make[1]: *** [CMakeFiles/Makefile2:15037: projects/compiler-rt/lib/lsan/CMakeFiles/clang_rt.lsan_osx_dynamic.dir/all] Error 2
make: *** [Makefile:152: all] Error 2
builder for ‘/nix/store/1fhimwr4ay7378y1hcrp92jgxgb5sj1m-llvm-5.0.0.drv’ failed with exit code 2

@dtzWill I don't have a problem with switching to clang-5 for 17.09 if everything looks ok, but the release managers should probably decide on that. Either way I can push a clang-5 stdenv to my wip jobset to see what breaks.

dtzWill · 2017-09-19T18:19:58Z

Looks like something in the llvm build is trying to codesign on darwin.

Pushed a commit that hopefully patches out that behavior, please let me know if this fixes the problem.

LnL7 · 2017-09-20T17:31:57Z

@dtzWill The darwin build looks good now.

LnL7 · 2017-09-26T21:14:41Z

@dtzWill can you squash this

dtzWill · 2017-09-26T21:37:12Z

Sure thing. Rebuilding after squash+rebase to ensure all is well, will push in a bit.

…

On Tue, Sep 26, 2017 at 4:14 PM, Daiderd Jordan ***@***.***> wrote: @dtzWill <https://github.com/dtzwill> can you squash this — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#29541 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAx4sjHpE16j360uhmLjhQxXUchCE2FDks5smWlKgaJpZM4PbepQ> .

dtzWill · 2017-09-27T01:25:45Z

(ready to go, I think! 😁)

LnL7 · 2017-09-27T20:18:10Z

pkgs/development/compilers/llvm/5/clang/default.nix

+}:
+
+let
+  gcc = if stdenv.cc.isGNU then stdenv.cc.cc else stdenv.cc.cc.gcc;


Just noticed this, cc.cc.gcc doesn't exist on other cc wrappers. Since it's only referenced on linux I think it's better to just inline it.

LnL7 · 2017-09-27T20:22:19Z

pkgs/development/compilers/llvm/5/clang/default.nix

+      "-DSPHINX_WARNINGS_AS_ERRORS=OFF"
+    ]
+    # Maybe with compiler-rt this won't be needed?
+    ++ stdenv.lib.optional stdenv.isLinux "-DGCC_INSTALL_PREFIX=${gcc}"


using stdenv.cc.isGNU might be a little more correct here.

dtzWill · 2017-10-01T14:39:49Z

Sorry for the delay, thanks for the comments.

I believe the expression is done this way to handle the case on Linux where clang is used to build clang: it still needs access to "some" gcc for glibc/stdlibc++ (wrapper probably handles this) but also for the support libraries like libgcc and platform bits like crtbegin.o.

I'm not sure if the cc-wrapper is able to reasonably guide clang to those resources, and if it can it might be nice to have the paths given reasonable default values for when using libclang or similar?

If there's a clear solution--or a motivated party to sort this out-- I'm all for the change, but otherwise perhaps it's okay as-is 😇

LnL7 · 2017-10-01T15:29:43Z

To clarify, I'm just talking about inlining the gcc variable since the else case is wrong and never used.

dtzWill · 2017-10-01T18:20:38Z

It's used when using clang to build clang, see clangSelf for an example: try adding an assert false to the else clause and you'll see it trigger (if you change llvmPackagesSelf to llvmPackages_5, or change the 3.4 clang's else clause instead).

LnL7 · 2017-10-01T18:40:37Z

Ah! In that case the gcc attribute would exist, still kind of confusing but let's just keep it for now then.

dtzWill · 2017-10-01T18:45:34Z

Definitely still kinda confusing, haha agreed :/.

dtzWill mentioned this pull request Sep 18, 2017

llvm: 4.0.1 -> 5.0.0rc2 #28410

Closed

8 tasks

Mic92 reviewed Sep 19, 2017

View reviewed changes

LnL7 approved these changes Sep 20, 2017

View reviewed changes

llvm: init 5.0

fb7ebf3

dtzWill force-pushed the update/llvm-5.0 branch from 91e2870 to fb7ebf3 Compare September 26, 2017 23:13

LnL7 reviewed Sep 27, 2017

View reviewed changes

LnL7 merged commit 4da8fd7 into NixOS:master Oct 1, 2017

LnL7 added the 8.has: port to stable A PR already has a backport to the stable release. label Oct 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

llvm: init 5.0 #29541

llvm: init 5.0 #29541

dtzWill commented Sep 18, 2017

andrewrk commented Sep 18, 2017

LnL7 commented Sep 18, 2017

Mic92 Sep 19, 2017

dtzWill Sep 19, 2017

Mic92 Sep 19, 2017

Mic92 Sep 19, 2017 •

edited

dtzWill Sep 19, 2017

Mic92 Sep 19, 2017

dtzWill commented Sep 19, 2017

dtzWill commented Sep 19, 2017

dtzWill commented Sep 19, 2017

andrewrk commented Sep 19, 2017 •

edited

LnL7 commented Sep 19, 2017

dtzWill commented Sep 19, 2017

LnL7 commented Sep 20, 2017

LnL7 commented Sep 26, 2017

dtzWill commented Sep 26, 2017 via email

dtzWill commented Sep 27, 2017

LnL7 Sep 27, 2017

LnL7 Sep 27, 2017

dtzWill commented Oct 1, 2017

LnL7 commented Oct 1, 2017

dtzWill commented Oct 1, 2017

LnL7 commented Oct 1, 2017

dtzWill commented Oct 1, 2017

llvm: init 5.0 #29541

llvm: init 5.0 #29541

Conversation

dtzWill commented Sep 18, 2017

Motivation for this change

Things done

andrewrk commented Sep 18, 2017

LnL7 commented Sep 18, 2017

Mic92 Sep 19, 2017

Choose a reason for hiding this comment

dtzWill Sep 19, 2017

Choose a reason for hiding this comment

Mic92 Sep 19, 2017

Choose a reason for hiding this comment

Mic92 Sep 19, 2017 • edited

Choose a reason for hiding this comment

dtzWill Sep 19, 2017

Choose a reason for hiding this comment

Mic92 Sep 19, 2017

Choose a reason for hiding this comment

dtzWill commented Sep 19, 2017

dtzWill commented Sep 19, 2017

dtzWill commented Sep 19, 2017

andrewrk commented Sep 19, 2017 • edited

LnL7 commented Sep 19, 2017

dtzWill commented Sep 19, 2017

LnL7 commented Sep 20, 2017

LnL7 commented Sep 26, 2017

dtzWill commented Sep 26, 2017 via email

dtzWill commented Sep 27, 2017

LnL7 Sep 27, 2017

Choose a reason for hiding this comment

LnL7 Sep 27, 2017

Choose a reason for hiding this comment

dtzWill commented Oct 1, 2017

LnL7 commented Oct 1, 2017

dtzWill commented Oct 1, 2017

LnL7 commented Oct 1, 2017

dtzWill commented Oct 1, 2017

Mic92 Sep 19, 2017 •

edited

andrewrk commented Sep 19, 2017 •

edited