gcc: deduplicate top-level expressions #243607
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
commented
Jul 15, 2023
pkgs/top-level/all-packages.nix
Outdated
Comment on lines
15429
to
15431
| else if atLeast "6" then (if stdenv.targetPlatform.isRedox then isl_0_17 else isl_0_14) | ||
| else if atLeast "4.9" then isl_0_11 | ||
| else /* "4.8" */ isl_0_14; |
There was a problem hiding this comment.
The ISL version we are using jumps backward from 0.14 on gcc48 to 0.11 on gcc49, then forward again at gcc6. If gcc49 cannot use isl 0.14 why is gcc48 able to? I doubt this is right, but if it is there should be an explanatory comment, and I found none.
pkgs/top-level/all-packages.nix
Outdated
Comment on lines
15428
to
15429
| else if atLeast "7" then isl_0_17 | ||
| else if atLeast "6" then (if stdenv.targetPlatform.isRedox then isl_0_17 else isl_0_14) |
There was a problem hiding this comment.
gcc6 bumps to isl 0.17 only if compiling for RedoxOS. This probably means that gcc6 is able to use isl 0.17 everywhere. If not, there should have been a comment explaining why RedoxOS is special here.
pkgs/top-level/all-packages.nix
Outdated
Comment on lines
15435
to
15436
| # Build fails on Darwin with clang | ||
| stdenv = if stdenv.isDarwin then gccStdenv else stdenv; |
There was a problem hiding this comment.
gcc49 on Darwin uses gccStdenv because "build fails on Darwin with clang"; surely if that is the case then that is true for gcc48 as well, no?
pkgs/top-level/all-packages.nix
Outdated
Comment on lines
15440
to
15441
| else if atLeast "4.9" then cloog_0_18_0 | ||
| else /* 4.8 */ cloog; |
There was a problem hiding this comment.
gcc49 overrides cloog to v0.18.0, but gcc48 doesn't. Again, very weird, probably somebody meant to override gcc 4.9 and all older versions, not just that one particular version.
ofborg
bot
added
10.rebuild-darwin: 0
ghost
marked this pull request as draft
This comment was marked as resolved.
This comment was marked as resolved.
ghost
marked this pull request as ready for review
ghost
commented
Jul 15, 2023
pkgs/top-level/all-packages.nix
Outdated
Comment on lines
15442
to
15444
| } // lib.optionalAttrs (atLeast "6" && !(atLeast "9")) { | ||
| # gcc 10 is too strict to cross compile gcc <= 8 | ||
| stdenv = if (stdenv.targetPlatform != stdenv.buildPlatform) && stdenv.cc.isGNU then gcc7Stdenv else stdenv; |
There was a problem hiding this comment.
when cross-compiling, gcc6,7,8 override stdenv to gcc7Stdenv because "gcc 10 is too strict to cross compile gcc <= 8" yet this override was not applied to any other versions of gcc which are "<= 8". It's also odd that gcc7Stdenv is used rather than gcc8Stdenv.
ofborg
bot
added
10.rebuild-linux: 0
ghost
mentioned this pull request
25 tasks
tjni
approved these changes
Jul 17, 2023
This commit deduplicates the copy-pasted gcc mess in pkgs/top-level/all-packages.txt, replacing it with a single expression that branches based on the chosen version. This refactoring has exposed several bizzare behaviors in our historical gcc expressions. These are almost certainly a result of people reaching back and making a change only to the version they care about, and neglecting adjacent versions. These oddities went un-noticed because in copy-paste-duplicated form it is nearly impossible to notice what has happened. When refactored into branch-on-version form, the irregularities jump out at you: 1. The ISL version we are using jumps *backward* from 0.14 on gcc48 to 0.11 on gcc49, then forward again at gcc6. If gcc49 cannot use isl 0.14 why is gcc48 able to? I doubt this is right, but if it is there should be an explanatory comment, and I found none. 2. gcc6 bumps to isl 0.17 only if compiling for RedoxOS. This probably means that gcc6 is able to use isl 0.17 everywhere. If not, there should have been a comment explaining why RedoxOS is special here. 3. gcc49 on Darwin uses gccStdenv because "build fails on Darwin with clang"; surely if that is the case then that is true for gcc48 as well, no? 4. gcc49 overrides cloog to v0.18.0, but gcc48 doesn't. Again, very weird, probably somebody meant to override gcc 4.9 *and all older versions*, not just that one particular version. 5. when cross-compiling, gcc6,7,8 override stdenv to gcc7Stdenv because "gcc 10 is too strict to cross compile gcc <= 8" yet this override was not applied to any other versions of gcc which are "<= 8". It's also odd that gcc7Stdenv is used rather than gcc8Stdenv. The best part about switch-on-version form is that oversights like the above *make the code more verbose* so they stand out. Fixing these likely-bugs will make the code simpler and shorter. In copy-paste-duplicated style the opposite is true: you get shorter code by *forgetting* to apply changes to all the versions that need the change! That's a very perverse incentive. This PR does not attempt to change or fix any of these behaviors. I have confirmed that it does not affect eval (same drv-hash) for all gcc versions on both x86_64-linux and x86_64-darwin.
|
Fixed merge conflict. |
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit does not affect eval (no drv-hash changes).
Description of changes
This commit deduplicates the copy-pasted gcc mess in
pkgs/top-level/all-packages.txt, replacing it with a single expression that branches based on the chosen version.This exposed several bizzare behaviors in our historical gcc expressions (these bullet points are repeated below alongside the code they refer to):
The ISL version we are using jumps backward from 0.14 on gcc48 to 0.11 on gcc49, then forward again at gcc6. If gcc49 cannot use isl 0.14 why is gcc48 able to? I doubt this is right, but if it is there should be an explanatory comment, and I found none.
gcc6 bumps to isl 0.17 only if compiling for RedoxOS. This probably means that gcc6 is able to use isl 0.17 everywhere. If not, there should have been a comment explaining why RedoxOS is special here.
gcc49 on Darwin uses gccStdenv because "build fails on Darwin with clang"; surely if that is the case then that is true for gcc48 as well, no?
gcc49 overrides cloog to v0.18.0, but gcc48 doesn't. Again, very weird, probably somebody meant to override gcc 4.9 and all older versions, not just that one particular version.
when cross-compiling, gcc6,7,8 override stdenv to gcc7Stdenv because "gcc 10 is too strict to cross compile gcc <= 8" yet this override was not applied to any other versions of gcc which are "<= 8". It's also odd that gcc7Stdenv is used rather than gcc8Stdenv.
The best part about switch-on-version form is that oversights like the above make the code more verbose so they stand out. Fixing these likely-bugs will make the code simpler and shorter. In copy-paste-duplicated style the opposite is true: you get shorter code by forgetting to apply changes to all the versions that need the change! That's a perverse incentive.
This PR does not attempt to change or fix any of these behaviors. I have confirmed that it does not affect eval (same drv-hash) for all gcc versions on both
x86_64-linuxandx86_64-darwin.Things done