Generic library rules #571
Conversation
|
Try running with |
|
@ndmitchell Thanks for the tip! In this particular case though it doesn't seem to unlock a more precise description: $ hadrian/build.sh -c -j1 --trace
Up to date
Up to date
[hadrian/cfg/system.cfg, settings, mk/config.h]
about to need ["hadrian/cfg/system.config.in","settings.in","mk/config.h.in"]
about to run ./configure
in doWith
builder ready
shakeArgsWith 0.000s 0%
Function shake 0.315s 80% =========================
Database read 0.000s 0%
With database 0.000s 0%
Running rules 0.077s 19% ======
Pool finished (1 threads, 1 max) 0.000s 0%
Total 0.393s 100%
Build system error - indirect recursion detected:
Key value 1: OracleQ (KeyValue ("hadrian/cfg/system.config","host-os"))
Key value 2: hadrian/cfg/system.config
Key value 3: hadrian/cfg/system.config settings mk/config.h
Key value 4: OracleQ (KeyValue ("hadrian/cfg/system.config","project-version"))
Rules may not be recursiveNow, if we carefully look at the error, we see that hadrian wants the config file for the The debugging output I added suggests that hadrian errors out when executing this line: doWith :: (Builder b, ShakeValue c)
=> (b -> BuildInfo -> Action a)
-> (Target c b -> Action ())
-> [(Resource, Int)] -> [CmdOption] -> Target c b -> Args c b -> Action a
doWith f info rs opts target args = do
putLoud "in doWith"
needBuilder (builder target)
putLoud "builder ready"
argList <- interpret target args -- /!\ HERE /!\
putLoud $ "args: " ++ show argList
trackArgsHash target
putLoud "args hash tracked"
info target
putLoud "target info printed"
verbose <- interpret target verboseCommand
putLoud "target interpreted"
let quietlyUnlessVerbose = if verbose then withVerbosity Loud else quietly
quietlyUnlessVerbose $ f (builder target) $
BuildInfo { buildArgs = argList
, buildInputs = inputs target
, buildOutputs = outputs target
, buildOptions = opts
, buildResources = rs } |
|
@alpmestan Thanks for the PR! I'm running from one meeting to another today, will hopefully find more time to have a look at it later today :) |
|
Alright, I also changed the call to Now, let's see how the travis tests go. |
|
@alpmestan Looks like we still have some cyclic rules failure on AppVeyor: Full build log: https://ci.appveyor.com/project/snowleopard/hadrian/build/1.0.1076 |
|
This isn't a cyclic value error, it's overlapping rules - way easier to fix. Just find which patterns match and adjust one. |
|
@ndmitchell Ah, indeed! |
|
@snowleopard Alright, the selftest + the 3 builds that we do on travis all succeed! Modulo that weird error at the end (
I'll look into this. |
|
OK, trying something for the error you mentionned above. See last commit. |
@alpmestan Indeed! 🎉 After the fix in #574 this will hopefully succeed on AppVeyor as well :) |
|
Woooohooooooo, all builds are green on travis. 🎉 Let's see what appveyor says. |
|
End of the build log on appveyor: Should we not |
|
Woohoo! Indeed, looks like we need to fix ApoVeyor script. Would you like to do that? |
|
Windows requires |
Sure. It's just weird that we have to remove |
|
Did the appveyor build get killed? |
src/Rules/Library.hs
Outdated
| asuf <- libsuf way | ||
| let isLib0 = ("//*-0" ++ asuf) ?== archivePath | ||
| removeFile archivePath | ||
| if isLib0 |
There was a problem hiding this comment.
There was a problem hiding this comment.
Done in upcoming commit.
| -- @a@, which represents that @something@, is instantiated as 'LibA', 'LibDyn' | ||
| -- and 'LibGhci' successively in this module, depending on the type of library | ||
| -- we're giving the build rules for. | ||
| data BuildPath a |
There was a problem hiding this comment.
Presumably someday this will become a generic piece of infastructure and will live elsewhere, but no need to move things right now. I'm just commenting that this is potentially way too valuable to be hidden in Rules.Libarary :)
There was a problem hiding this comment.
Right, I find this rather useful. I too like the parser-based approach combined with things like BuildPath, I didn't move it to a dedicated module and what not because for now we're just using it there, but I was curious to see whether we would like to do something similar "everywhere we can". Glad to see the positive feedback.
|
@alpmestan I've left a couple of minor comments, but overall this PR looks great, thank you! I'm looking forward to simplifying other build rules following the same approach :) |
alpmestan
force-pushed
the
generic-library-rules
branch
from
30ede7d
to
b3b8f55
Compare
|
Hmm, the appveyor build fails with: |
|
@alpmestan I guess this failure could be fixed after #564, so let's not bother about it for now. |
|
@snowleopard I think I'm done addressing your feedback. Anything else you want me to do on this patch? |
|
@alpmestan No, looks great -- merged. Thanks again :) |
Previously, as reported in #15938, resuming a build "in the middle", e.g when building _build/stage1/libraries/base/, hadrian would take up to a whole minute to get started doing actual work, building code. This was mostly due to a big enumeration that we do in Rules.hs, to generate all the possible patterns for object files for 1) all ways, 2) all packages and 3) all stages. Since rule enumeration is always performed, whatever the target, we were always paying this cost, which seemed to grow bigger the farther in the build we stopped and were resuming from. Instead, this patch borrows the approach that we took for Rules.Library in snowleopard/hadrian#571, which exposes all the relevant object files under as few catch-all rules as possible (8 here), and parses all the information we need out of the object's path. The concrete effect of this patch that I have observed is to reduce the 45-60 seconds pause to <5 seconds. Along with the Shake performance improvements that Neil mentions in #15938, most of the pause should effectively disappear.
Summary: Previously, as reported in #15938, resuming a build "in the middle", e.g when building _build/stage1/libraries/base/, hadrian would take up to a whole minute to get started doing actual work, building code. This was mostly due to a big enumeration that we do in Rules.hs, to generate all the possible patterns for object files for 1) all ways, 2) all packages and 3) all stages. Since rule enumeration is always performed, whatever the target, we were always paying this cost, which seemed to grow bigger the farther in the build we stopped and were resuming from. Instead, this patch borrows the approach that we took for Rules.Library in snowleopard/hadrian#571, which exposes all the relevant object files under as few catch-all rules as possible (8 here), and parses all the information we need out of the object's path. The concrete effect of this patch that I have observed is to reduce the 45-60 seconds pause to <5 seconds. Along with the Shake performance improvements that Neil mentions in #15938, most of the pause should effectively disappear. Test Plan: Reviewers: snowleopard Subscribers: GHC Trac Issues: Trac #15938
Summary: Previously, as reported in #15938, resuming a build "in the middle", e.g when building _build/stage1/libraries/base/, hadrian would take up to a whole minute to get started doing actual work, building code. This was mostly due to a big enumeration that we do in Rules.hs, to generate all the possible patterns for object files for 1) all ways, 2) all packages and 3) all stages. Since rule enumeration is always performed, whatever the target, we were always paying this cost, which seemed to grow bigger the farther in the build we stopped and were resuming from. Instead, this patch borrows the approach that we took for Rules.Library in snowleopard/hadrian#571, which exposes all the relevant object files under as few catch-all rules as possible (8 here), and parses all the information we need out of the object's path. The concrete effect of this patch that I have observed is to reduce the 45-60 seconds pause to <5 seconds. Along with the Shake performance improvements that Neil mentions in #15938, most of the pause should effectively disappear. Test Plan: Reviewers: snowleopard Subscribers: GHC Trac Issues: Trac #15938
Summary: Previously, as reported in #15938, resuming a build "in the middle", e.g when building _build/stage1/libraries/base/, hadrian would take up to a whole minute to get started doing actual work, building code. This was mostly due to a big enumeration that we do in Rules.hs, to generate all the possible patterns for object files for 1) all ways, 2) all packages and 3) all stages. Since rule enumeration is always performed, whatever the target, we were always paying this cost, which seemed to grow bigger the farther in the build we stopped and were resuming from. Instead, this patch borrows the approach that we took for Rules.Library in snowleopard/hadrian#571, which exposes all the relevant object files under as few catch-all rules as possible (8 here), and parses all the information we need out of the object's path. The concrete effect of this patch that I have observed is to reduce the 45-60 seconds pause to <5 seconds. Along with the Shake performance improvements that Neil mentions in #15938, most of the pause should effectively disappear. Test Plan: Reviewers: snowleopard Subscribers: GHC Trac Issues: Trac #15938
Summary: Previously, as reported in #15938, resuming a build "in the middle", e.g when building _build/stage1/libraries/base/, hadrian would take up to a whole minute to get started doing actual work, building code. This was mostly due to a big enumeration that we do in Rules.hs, to generate all the possible patterns for object files for 1) all ways, 2) all packages and 3) all stages. Since rule enumeration is always performed, whatever the target, we were always paying this cost, which seemed to grow bigger the farther in the build we stopped and were resuming from. Instead, this patch borrows the approach that we took for Rules.Library in snowleopard/hadrian#571, which exposes all the relevant object files under as few catch-all rules as possible (8 here), and parses all the information we need out of the object's path. The concrete effect of this patch that I have observed is to reduce the 45-60 seconds pause to <5 seconds. Along with the Shake performance improvements that Neil mentions in #15938, most of the pause should effectively disappear. Test Plan: Reviewers: snowleopard Subscribers: GHC Trac Issues: #15938
Summary: Previously, as reported in #15938, resuming a build "in the middle", e.g when building _build/stage1/libraries/base/, hadrian would take up to a whole minute to get started doing actual work, building code. This was mostly due to a big enumeration that we do in Rules.hs, to generate all the possible patterns for object files for 1) all ways, 2) all packages and 3) all stages. Since rule enumeration is always performed, whatever the target, we were always paying this cost, which seemed to grow bigger the farther in the build we stopped and were resuming from. Instead, this patch borrows the approach that we took for Rules.Library in snowleopard/hadrian#571, which exposes all the relevant object files under as few catch-all rules as possible (8 here), and parses all the information we need out of the object's path. The concrete effect of this patch that I have observed is to reduce the 45-60 seconds pause to <5 seconds. Along with the Shake performance improvements that Neil mentions in #15938, most of the pause should effectively disappear. Reviewers: snowleopard, bgamari Subscribers: rwbarton, carter GHC Trac Issues: #15938 Differential Revision: https://phabricator.haskell.org/D5412
Previously, as reported in #15938, resuming a build "in the middle", e.g when building _build/stage1/libraries/base/, hadrian would take up to a whole minute to get started doing actual work, building code. This was mostly due to a big enumeration that we do in Rules.hs, to generate all the possible patterns for object files for 1) all ways, 2) all packages and 3) all stages. Since rule enumeration is always performed, whatever the target, we were always paying this cost, which seemed to grow bigger the farther in the build we stopped and were resuming from. Instead, this patch borrows the approach that we took for Rules.Library in snowleopard/hadrian#571, which exposes all the relevant object files under as few catch-all rules as possible (8 here), and parses all the information we need out of the object's path. The concrete effect of this patch that I have observed is to reduce the 45-60 seconds pause to <5 seconds. Along with the Shake performance improvements that Neil mentions in #15938, most of the pause should effectively disappear. Reviewers: snowleopard, bgamari, goldfire Reviewed By: snowleopard Subscribers: rwbarton, carter GHC Trac Issues: #15938 Differential Revision: https://phabricator.haskell.org/D5412
This patch implements the suggestion by @angerman in #538 -- to "register" the rules for building libraries (.a, .so, .dylib, etc) without having to parse any Cabal file. This dramatically decreases the "boot" time of hadrian and will, as soon as we address the problem I describe below, allow us to use the
-coption again.Instead of defining a whole bunch of precise rules for
<build root>/stage<N>/<path/to/lib/in/ghc/tree>/build/libHS...for each package, we just define catch-all rules for each type of library (extension, really) and use simpleparsecparsers (parsecis already a dependency, throughCabal, since ghc 8.4.1 / Cabal 2.2) to deconstruct the path and basically turn it into a suitable libraryContext.The remaining issue: I still seem to be able to build a GHC just fine if I run the
./boot && ./configurestep myself explicitly, but not if I start from a clean source tree and run hadrian with-c. The error I get in that case is:(the first few lines come from debugging output I added -- see the patch)
Does anyone have any idea of where that cycle comes from and why my patch makes this happen?