module dependency analysis improvements #543
Conversation
shayne-fletcher
force-pushed
the
calc-module-deps-no-ignore-pkgs
branch
3 times, most recently
from
9bddc52
to
c77c75a
Compare
shayne-fletcher
force-pushed
the
calc-module-deps-no-ignore-pkgs
branch
3 times, most recently
from
32a3330
to
2f7a46f
Compare
shayne-fletcher
changed the title
shayne-fletcher
force-pushed
the
calc-module-deps-no-ignore-pkgs
branch
5 times, most recently
from
36381b3
to
96a68d4
Compare
shayne-fletcher
force-pushed
the
calc-module-deps-no-ignore-pkgs
branch
from
96a68d4
to
ee4dd71
Compare
|
@samuel-williams-da phew! that was a marathon 😄 this is finally ready to go. the last of the big changes. |
|
@nycnewman Since this adds new jobs, we'll want to add these also to the required checks |
|
Thanks a bunch for all this work @shayne-fletcher! I'll take a look tomorrow morning :) |
|
A quick question of curiosity - I see you're now testing 9.0.2, what metric are you using to decide which versions need explicit tests? |
i'm working towards having a test of each series: 8.8, 8.10, 9.0, 9.2, 9.4., 9.6, 9.8, 9.10. according to that metric we are currently only lacking tests for (flavors) |
|
Ah right okay, good to know - thanks! |
i expect now this volley of work is out of the way this list will be static and only extended on new ghc releases (next being ghc-9.12) so once a year? on the other hand, we could, add at the cost of some complexity, fold all of these jobs into a single workflow if that makes any difference? in short, i'm open to rephrasing the workflows to simplify the repo config mgt if that helps. |
|
If GA doesn't have something simple for this, I'd rather leave as is, I'd still like to easily know which part has failed. |
morning @samuel-williams-da ! how's it looking? ok to move forward on this one? |
There was a problem hiding this comment.
I'm not entirely sure how you come to the changes needed for the variouus Main.hs files, but they seem fine. It would be helpful to arrange a walk through at some point so we can be on the same page as you when reviewing this code, we may reach out more formally about this after some discussion. (cc: @dylant-da)
| copyFile file new_p | ||
|
|
||
| calcModuleDeps :: [FilePath] -> [FilePath] -> [FilePath] -> GhcFlavor -> FilePath -> String -> String | ||
| calcModuleDeps includeDirs _hsSrcDirs hsSrcIncludes ghcFlavor cabalPackageDb ghcMakeModeOutputFile = |
There was a problem hiding this comment.
I see quite a few options got removed here, why are they not needed anymore?
There was a problem hiding this comment.
this is the exciting part for me. the fact is that over the course of these last 5 years through all the versions we've supported and the complexity of the procedure as conducted via stack we accumulated a lot of workarounds and hacks that we determined were necessary for the time (and in truth often with a less than perfect understanding of what/why "find a way to keep it working"... "do what you gotta do" ...). now with stack removed and reliance strictly on the cabal solver i've gone back and sought opportunities to remove these things. this is the result!
There was a problem hiding this comment.
to be more explicit about what we've removed exactly here though: the preprocessor constants STAGE=2, GHC_IN_GHCI don't influence the outcome of the ghc -M. they probably never did and have always been redundant. the constant GHCI determines if ghc -M includes GHCi.Run and since the absence of that module does not affect the behavior of ghc-lib-mini-compile-test i judge it ok to exclude that module and drop the constant (less code is better). the ignore-package directives were found to be unnecessary across all combinations tested so far (!) and so could be removed, similarly the -package exceptions has yet to be proven to be needed there (perhaps there's a config we haven't encountered yet that will require adding it back - we'll see [in contrast, i confirm that there's no removing the semaphore-compat package from this list]). the semaphoreCompatBootExists and ghcPackagePath related code got refactored into a better form using the clear-package-db, ..., package-db flags.
There was a problem hiding this comment.
Oh nice! That is exciting! Happy to see simplifications here :)
these files are just getting their import lists ordered alphabetically |
Co-authored-by: Samuel Williams <[email protected]>
i should be delighted to meet with you and walk through the role and meaning of these files and indeed anything else! |
in a nutshell, these |
|
@samuel-williams-da i know these reviews have been hard work. i just want to say thanks and for you to know that i appreciate you. |
|
@samuel-williams-da @dylant-da might this now get merged please? |
|
Apologies, Friday was a company holiday - thanks for this work! I'll merge now |
simplify ghc -M and add new tests (in total now 6 x 3 builds).