-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
cc-wrapper: deunify clang/gcc treatment of -isystem #225220
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Hrm, it got farther but failed (differently). Investigating.
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
@amjoseph-nixpkgs i think you want to target staging-next? |
I think I'll go for a workaround using this just for the specific breakage(s) in the current staging-next, as rebuilding linux stdenvs is quite costly. |
This causes a global rebuild; is that okay? I thought Anyways it doesn't matter to me. Also, my rebuild went way faster than I thought; I should have a rebuilt-from-scratch |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
I'm a bit confused that you mention |
(it was |
No problem :-) Fortunately it still seems like simple hacks will suffice for now, so a proper solution isn't urgent and could be a mass rebuild. |
✔️ This PR is ready. @vcunat @trofi please consider merging this. It contains two commits, and both are needed. If you omit either one of them, Got it! To fix Proof:
scipy fails to build if you omit cc-wrapper: deunify clang/gcc treatment of -isystemnix@moore:/git/work/fix/scikit/with-trofi-idea$ git log | head -n 8 commit 9d5d1f7cb3aa329b2029b0cdf97334b26b4118e9 Author: Adam Joseph Date: Sat Apr 8 10:54:19 2023 -0700
nix@moore:/git/work/fix/scikit/with-trofi-idea$ nix build -f . -L python3Packages.scikit-learn scikit-learn fails to build if you omit gcc: never disableBootstrap for gfortrannix@moore:/git/work/fix/scikit/with-trofi-idea$ git reset --hard de7cbf714b8562d4dddbe284a205e5d92f7b7bf7 HEAD is now at de7cbf714b85 cc-wrapper: deunify clang/gcc treatment of -isystem nix@moore:/git/work/fix/scikit/with-trofi-idea$ git revert HEAD^ [with-trofi-idea 797d9b6aaad1] Revert "gcc: never disableBootstrap for gfortran" 1 file changed, 1 insertion(+), 4 deletions(-) nix@moore:/git/work/fix/scikit/with-trofi-idea$ nix-build . -A python3Packages.scikit-learn ... > gcc -shared -L/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-zlib-1.2.13/lib -L/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-bzip2-1.0.8/lib -L/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-expat-2.5.0/lib -L/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-xz-5.4.2/lib -L/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-libffi-3.4.4/lib -L/nix/store/2scdlrgd8sqmm1sgsg9b7sjnwlmr9s1p-libxcrypt-4.4.33/lib -L/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-gdbm-1.23/lib -L/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-sqlite-3.41.2/lib -L/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-readline-8.2p1/lib -L/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-ncurses-6.4/lib -L/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-openssl-3.0.8/lib -L/nix/store/1whrncscgc90j7dh08wcyvygjgzcjkgm-tzdata-2023c/lib -L/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-zlib-1.2.13/lib -L/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-bzip2-1.0.8/lib -L/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-expat-2.5.0/lib -L/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-xz-5.4.2/lib -L/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-libffi-3.4.4/lib -L/nix/store/2scdlrgd8sqmm1sgsg9b7sjnwlmr9s1p-libxcrypt-4.4.33/lib -L/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-gdbm-1.23/lib -L/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-sqlite-3.41.2/lib -L/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-readline-8.2p1/lib -L/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-ncurses-6.4/lib -L/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-openssl-3.0.8/lib -L/nix/store/1whrncscgc90j7dh08wcyvygjgzcjkgm-tzdata-2023c/lib build/temp.linux-x86_64-cpython-310/sklearn/_loss/_loss.o -L/nix/store/xasxsk7n57j3mx50vz848gd04i95kj4v-python3-3.10.11/lib -Lbuild/temp.linux-x86_64-cpython-310 -lm -llibsvm-skl -lliblinear-skl -o build/lib.linux-x86_64-cpython-310/sklearn/_loss/_loss.cpython-310-x86_64-linux-gnu.so -fopenmp > error: command '/nix/store/02nzdnalvzi8mf6p2l8svl6swy9bp5by-gfortran-wrapper-12.2.0/bin/gcc' failed with exit code 1 > /nix/store/fqvpf04307g05fibxjg287z5cga3pcd9-stdenv-linux/setup: line 1594: pop_var_context: head of shell_variables not a function context For full logs, run 'nix log /nix/store/0khrvashssrr1rr7z0cyx8b77xjiyy7a-python3.10-scikit-learn-1.2.1.drv'. |
This comment was marked as outdated.
This comment was marked as outdated.
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, to state the obvious it's a full linux rebuild:
$ ./maintainers/scripts/rebuild-amount.sh HEAD^
Estimating rebuild amount by counting changed Hydra jobs (parallel=unset).
1 pkgs-lib-tests
2 x86_64-darwin
57773 x86_64-linux
Yes indeed.
|
I have cherry-picked 4b9280f8fd008b7bd22aeff50ede5195dcf355fb and then reverted it so we won't forget to do so. Once |
Needs rebase |
Well yes, it needs a revert of e269122. |
i did
then added
then ran and i think it worked
|
Again, temporary until PR #225220
BTW thanks for telling me about this trick earlier. I figured out how to do it without manually counting commits (I often rebase 100+-long branches):
and then you can do things like
|
This reverts commit 398b54b.
This reverts commit e269122.
This reverts commit 46f29d4.
As suggested by @trofi here: #209870 (comment) This should fix failures among packages which use gfortran: #209870 (comment) https://hydra.nixos.org/build/215195834
In #209870 I tried to unify the treatment of clang and gcc in cc-wrapper as much as possible. However it appears that I went too far. Clang requires -isystem flags in order to be able to find gcc's libstdc++. Gcc does not need these flags. If they are added, gfortran will get confused: #209870 (comment) This commit deunifies the chunk of code that adds the -isystem flags, and explains why this chunk applies only to clang.
added another revert |
Problem
Failures reported among packages which use gfortran:
Methodology
-v
toNIX_CFLAGS_COMPILE
python3Packages.scipy
with and without #209870 revertednuke-refs
on the resulting logswdiff -n | colordiff
on the nuked logsDiagnosis
The fix
Validation
I hacked in this PR's fix to
scipy
(only) usingwrapCCWith
andscipy
built correctly. Currently verifying a global rebuild with this PR, but it won't finish until after I go to sleep.@vcunat, if the situation is urgent we can send the☹️
wrapCCWith
-hacked version tostaging-next
. Otherwise the fix has to wait for this PR's global rebuild