-
-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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_{7-12}.compiler-rt: install resource files to DATADIR #123103
llvmPackages_{7-12}.compiler-rt: install resource files to DATADIR #123103
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
|
I'd strongly suggest that we should first have some |
If we only touch (As a side note, seems like darwin stdenv doesn't link the resource files into the resource root at all). |
In the worst case we could make the change linux-only for now or something. |
Result of 28 packages marked as broken and skipped:
4 packages failed to build:363 packages skipped due to time constraints:
30 packages built successfully:
Note that build failures may predate this PR, and could be nondeterministic or hardware dependent. |
The linux builders should be able to chew through it in a day. Darwin will suffer greatly as it takes many days for it to complete a stdenv rebuild; and darwin has been delaying staging-next enough already. Delaying staging-next any further is now taking away from doing stabilization from the next staging-next. |
For linux, this only causes ~600 rebuilds |
Likely due to an oversight, after 7869d16 some of the resource files of compiler-rt are installed to $dev/include instead of $out/share which breaks the assumption of multiple cc wrappers in llvmPackages. To resolve this without causing a mass rebuild on darwin, link the files from the wrong location into the correct place in clang's resource root. The proper fix for this is NixOS#123103 which however causes a darwin stdenv rebuild and should go through staging first.
I'm trying to find a workaround which saves us from the darwin stdenv rebuild. If that doesn't pan out we'll have to have two different patches for darwin and linux I guess. In any case I would propose that we try to merge the proper fix (whatever it'll look like) with the next staging-next PR then? |
@GrahamcOfBorg build chromium |
Yikes :(. I'm sorry this one got in there. |
7869d16 changed how resource files are installed. Likely by accident, now some of the resource files are installed to $dev/include instead of $out/share. This causes the cc wrapper's resource-root to miss those files from compiler-rt as they are in a different place than expected. This commit fixes all instances of this incorrect installation for llvmPackages_10, 11 and 12 which are the only llvm package sets which link ${targetLlvmLibraries.compiler-rt.out}/share to the resource-root. For the other llvm package set this will likely also need to be fixed, but it doesn't have to have immediate urgency and doing it in two steps allows us to (hopefully) fix the chromium build without causing a darwin stdenv rebuild. The full fix can be found in NixOS#123103 and should probably be included in the next staging-next rotation.
Yea, not sure why I thought it was a good idea for ofborg to try and build chromium |
Since the reduced changes were merged into staging-next. Should this be rebased on staging, so that the stdenv rebuild can be done on the next staging-next? |
Sounds good to me. |
This is in an effort to fix the following build failure shown by chromium: clang++: error: no such file or directory: '/nix/store/fhd89wrmkx6nflzjk0d6waz70bk3zc4i-clang-wrapper-12.0.0/resource-root/share/cfi_blacklist.txt' As it turns out a change introduced via the gnu-install-dirs.patch caused `add_compiler_rt_resource_file` to install resource files to $dev/include (FULL_INCLUDEDIR) instead of $out/share (FULL_DATADIR) which in turn meant that the clang wrappers we had didn't link those files to its resource root at all. Alternative fix to this would have been to link compiler-rt.dev/include/*.txt to the wrappers resource-root/share as well, but since this was handled inconsistently across the patch anyways (the dfsan list is installed correctly), opt to handle this consistently within the patch. llvmPackages_{5,6} install the resource files to a completely different location and need separate investigation.
This is done for 10-12, but not for the earlier llvm package sets.
01971cf
to
74f709f
Compare
This is in an effort to fix the following build failure shown by
chromium:
As it turns out a change introduced via the gnu-install-dirs.patch
caused
add_compiler_rt_resource_file
to install resource files to$dev/include (FULL_INCLUDEDIR) instead of $out/share (FULL_DATADIR)
which in turn meant that the clang wrappers we had didn't link those
files to its resource root at all.
Alternative fix to this would have been to link
compiler-rt.dev/include/*.txt to the wrappers resource-root/share as
well, but since this was handled inconsistently across the patch anyways
(the dfsan list is installed correctly), opt to handle this
consistently within the patch.
llvmPackages_{5,6} install the resource files to a completely different
location and need separate investigation.
Motivation for this change
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)