-
Notifications
You must be signed in to change notification settings - Fork 37
[WIP] Support run GHC's test from hadrian. #495
Conversation
1. Necessary command line arguments to run test driver. + `--test-only=<TEST_CASE>` + `--test-skip-perf` + `--test-summary=<SUMMARY_FILE>` + `--test-junit=<SUMMARY_FILE>` + `--test-config=<EXTRA_TEST_CONFIG>` 2. Synchronize configurations from test.mk. 3. Synchronize GHC's compilation flags from test.mk (that's very important). Signed-off-by: HE, Tao <[email protected]>
Another question is that I have noticed that currently hadrian just call sytem's |
@sighingnow Many thanks for this. To answer your questions:
I think that's fine. Otherwise there is really no way to conveniently use the built-in
At the moment we run |
src/Hadrian/Utilities.hs
Outdated
@@ -453,3 +460,21 @@ renderUnicorn ls = | |||
ponyPadding = " " | |||
boxLines :: [String] | |||
boxLines = ["", "", ""] ++ (lines . renderBox $ ls) | |||
|
|||
-- | These arguments are used by the `test` target. | |||
data TestArgs = TestArgs |
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.
I'm not sure why TestArgs
is defined here, let's move it to src/CommandLine.hs
.
Everything in src/Hadrian
is supposed to be generally useful for build systems, i.e. not GHC-specific, whereas flags passed to the GHC's testsuite are very much GHC-specific.
src/Settings.hs
Outdated
@@ -66,3 +66,7 @@ findPackageByName name = find (\pkg -> pkgName pkg == name) knownPackages | |||
-- | Install's DESTDIR setting. | |||
getDestDir :: Action FilePath | |||
getDestDir = fromMaybe "" <$> cmdInstallDestDir | |||
|
|||
-- | Arguments to run GHC's test script. | |||
getTestArgs :: Action TestArgs |
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.
Not clear why need another name for the same function. I'd suggest to drop it.
src/Rules/Test.hs
Outdated
let timeout_prog = "testsuite/timeout/install-inplace/bin/timeout" | ||
|
||
quietly . cmd "python3" $ | ||
[ "testsuite/driver/runtests.py" ] ++ |
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.
We probably should be constructing this command line using the abstractions available in Hadrian. For example:
https://github.com/snowleopard/hadrian/blob/master/src/Settings/Builders/Haddock.hs
Thanks for your reply and review. I will fix these problem this weekends. I'll work on the |
I recently got started tackling the very same thing, but you went through much more trouble to integrate this properly with command-line flags etc. Can this run .T tests properly too, or more generally can we just run all tests and get the same result that we get with the I got started looking into this in the context of #445 but if you do most of the work, that would be great and I'll then just have to tweak a couple of things to make it work with that branch. So... thanks! :) |
@alpmestan aren't you also working on this? cc @bgamari |
@simonmar I am indeed, I left a comment above to ask a question or two. This patch is most likely saving me quite some work. |
@alpmestan Ah, hopefully there wasn't too much work duplication. Note that this PR grew out of solving a bug in the test rule: #494. Perhaps, we should create a more general issue to coordinate the work on test build rules in Hadrian. |
@snowleopard Well if this PR gets us to a point where we can run tests with the same results that we get with the make build system, then I don't think there's much more to do for now, hence my questions above :) I haven't done a lot of duplicate work, but I did some. It's not all lost though, as this allowed me to look at and understand the testing infra of GHC as well as more code in Hadrian. |
@alpmestan Currently I just have taken flags from Make hadrian's I'm willing to take the job to integrate hadrian to GHC's |
src/Rules/Test.hs
Outdated
, "-e", "config.in_tree_compiler=True" -- Use default value, see https://github.com/ghc/ghc/blob/master/testsuite/mk/boilerplate.mk | ||
|
||
, "--config-file=testsuite/config/ghc" | ||
, "--config", "compiler=" ++ show (top -/- compiler) |
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.
These two lines have been a problem for me, as the config file seems to take priority and it also defines a config.compiler
field. @sighingnow Can you confirm this is not the case for you and the correct GHC is picked?
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.
I have confirm that this line let me execute the right ghc
(inplace/bin/ghc-stage2) to compile the test.
Without this config, the ghc
in $PATH
will be called.
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.
OK, somehow the --config
options were not working for me, glad you got that to work!
src/Rules/Test.hs
Outdated
-- , "--config", "gs=$(call quote_path,$(GS))" | ||
-- , "--config", "timeout_prog=$(call quote_path,$(TIMEOUT_PROGRAM))" | ||
-- See: https://github.com/ghc/ghc/blob/master/testsuite/mk/test.mk#L291 | ||
let timeout_prog = "testsuite/timeout/install-inplace/bin/timeout" |
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.
Does this therefore rely on having the timeout prog make rules executed before running hadrian's test
rule? I do have a rule or two to handle the timeout program in my branch. I don't properly implement the Windows logic yet, I always pick the python script, but that's a start.
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.
If the timeout_prog
is not set, I get a [Error 13] Permission denied
in python script (inside the runCmd
method).
I'm working with Bash on Windows, it's a Ubuntu 16.04 environment.
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.
We have the following code in testlib.py
:
r = subprocess.Popen([timeout_prog, timeout, cmd],
stdin=stdin_file,
stdout=subprocess.PIPE,
stderr=hStdErr,
env=ghc_env)
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.
Right, so this indeed requires that you build it through the make build system first. I have a bit of code that works on Linux for wrapping the python version of the program in a shell script and using that for the tests. It's a bit ugly but it does the trick. You'd need to replicate the Windows logic to make this work on your Windows machine, but as you can see here it's not scary enough to prevent us from doing it all in hadrian, I think. Here's the code that I have for the non-Windows case (that is, the part that replicates this bit of the Makefile).
-- FIXME: add ".exe" extension on windows
timeoutProgPath :: FilePath
timeoutProgPath = "test" -/- "bin" -/- "timeout"
timeoutPyPath :: FilePath
timeoutPyPath = "test" -/- "bin" -/- "timeout.py"
-- and then in `testRules`...
root <- buildRootRules
root -/- timeoutPyPath ~> do -- FIXME: run only when not on Windows
copyFile "testsuite/timeout/timeout.py" (root -/- timeoutPyPath)
root -/- timeoutProgPath ~> do
need [ root -/- timeoutPyPath ]
let script = unlines
[ "#!/usr/bin/env sh"
, "exec python3 $0.py \"$@\""
]
liftIO $ do
writeFile (root -/- timeoutProgPath) script
cmd "chmod" [ "+x", root -/- timeoutProgPath ]
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.
If we can get ride of current python's test driver, we could do the timeout
directly in Haskell.
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.
I'm afraid the python test driver is not going anytime soon, and we might want to aim for a more reasonable goal of just replicating that tiny Makefile's logic in hadrian, so that we can at least properly build/wrap the appropriate timeout program but also need
it in any rule that will run tests. Really just replicating what's done in the makefile I linked to but in hadrian, so that we don't introduce any (implicit) dependency on them make build system for running the test rules.
@sighingnow I see! So we are both facing the same problem. One idea that I had yesterday to handle this problem without touching GHC's codebase at all might do the trick but requires going through a few hacky hoops. What if we copied over the makefiles for all the tests that have one, along with the files needed for the test, and put them somewhere under the build root while following the same hierarchy. Except that we could edit the makefiles on the fly. As far as I can tell, many (most? all?) makefiles define It sounds to me like such an approach could work and has the huge advantage that this doesn't require a single change to GHC's codebase. I'm not sure defining TEST_HC and what not on the spot is the best option, but I do think it is necessary to work with variations on those |
Yeah, we just need to remove the We could leave the |
Either that or define those variables in our modified
Right, but I think the makefiles rely on having the .hs (and others) files in the same directory, so we would probably have to move those over as well, without changing them though. Alright @sighingnow, are you willing to give a shot at implementing this or should I? |
I still don't think we need to move these files, before
Do you mean modify the test cases' I think we can pass necessary variables via system's environment variable. Define these variables in |
@alpmestan I'm really not familiar with hadrian's codebase so it would be better if you could submit such a patch :) I'll try to follow your work. |
Well, anything that gets all the tests to be executed properly, really :) If you want to implement a plan you have in mind, that's fine too! |
Just to clarify: I'm by no means an expert on Hadrian's code. I've just been working on/with it for several few weeks. I took the liberty to give some feedback and suggestions on ways to fix the test rules because I'm myself facing the same problem. I'm nowhere saying my suggestions are better, just trying to figure out a way to get hadrian to be a fine companion for running tests. |
@sighingnow @alpmestan I'm happy to be your go-to expert on Hadrian :) Feel free to submit half-cooked PRs for review and I'll suggest how to make the best use of Hadrian infrastructure and/or do some polishing after merging. Thank you for helping! |
@snowleopard For the record, @sighingnow and I had a little chat by email, and he's willing to take a stab at implementing something along the lines of what we have been suggesting above. By combining everyone's knowledge, fixes and experiments, we will get that testsuite running smoothly sooner or later :) (While you're around Andrey, I think it might be good that you and (Moritz and/or me) have a little discussion about #445 and how we should go about merging most/all of that work -- should I start an email thread? Sorry for the off-topic question on this PR.) |
@alpmestan Awesome! Sure, feel free to start an email thread to discuss #445. There is already a bit of a discussion in the PR itself, so let's include all participants. We could also setup a Skype chat to speed things up. |
Signed-off-by: HE, Tao <[email protected]>
Signed-off-by: HE, Tao <[email protected]>
Update summary:
Help wanted:
|
Regarding question number 1: the code so far looks reasonable. It is much nicer to have a proper "builder" for running the tests. If I were you I would not worry much more about that code at least until the tests all run properly. :) Regarding question number 2, I think @snowleopard or someone else will have a better answer. |
liftIO $ do | ||
setEnv "MAKE" makePath | ||
setEnv "TEST_HC" ghcPath | ||
setEnv "TEST_HC_OPTS" ghcFlags |
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.
We typically set the environment as follows:
https://github.com/snowleopard/hadrian/blob/master/src/Rules/Gmp.hs#L41-L44
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.
I have tried
-- Set environment variables for test's Makefile.
env <- sequence
[ builderEnvironment "MAKE" $ Make ""
, builderEnvironment "TEST_HC" $ Ghc CompileHs Stage2
, AddEnv "TEST_HC_OPTS" <$> runTestGhcFlags ]
-- Execute the test target.
buildWithCmdOptions env $ target (vanillaContext Stage2 compiler) RunTest [] []
But it doesn't work. The env variables $MAKE
, TEST_HC
and TEST_HC_OPTS
are still undefined when the python driver invoke the $MAKE
command.
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.
Hmm, strange. Thanks for leaving a TODO message, I also suggest to open an issue so we don't forget about this -- the ability to set an environment for a builder is important and we should figure out the best approach for it.
src/Rules/Test.hs
Outdated
build $ target (vanillaContext Stage2 compiler) RunTest [] [] | ||
|
||
timeoutPyPath :: FilePath | ||
timeoutPyPath = "test" -/- "bin" -/- "timeout.py" |
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.
In such cases we are usually more direct in Hadrian: test/bin/timeout.py
. As far as I know, this doesn't cause any compatibility issues across different OSs.
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.
Fixed.
src/Settings.hs
Outdated
@@ -66,3 +66,23 @@ findPackageByName name = find (\pkg -> pkgName pkg == name) knownPackages | |||
-- | Install's DESTDIR setting. | |||
getDestDir :: Action FilePath | |||
getDestDir = fromMaybe "" <$> cmdInstallDestDir | |||
|
|||
-- | Arguments to run GHC's test script. | |||
getTestArgs :: Args |
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.
Why is this defined here and not in Settings.Builders.RunTest
together with the rest of the flags?
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.
I define this method in src/Setting.hs
to translate the arguments from command line to Args
type.
Without this function I'll need to import module CommandLine
into Settings.Builders.RunTest
to do the transformation. I don't sure if it's the right way (import CommandLine
into the builder module).
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.
The getTestArgs
returns the arguments provided by user from command line. In Settings.Builders.RunTest
we only process the arguments decided by current GHC's configuration.
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.
We use Settings.Builders.X
to define all X-specific arguments. This seems to be relevant only for RunTest
.
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.
I don't sure if it's the right way (import CommandLine into the builder module).
Yes, that's fine, as long as this doesn't create nasty import cycles. In fact, we do something very similar in Settings.Builders.Make
where we rely on shakeThreads <$> expr getShakeOptions
which essentially comes from Hadrian's command line (but captured by Shake).
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.
I have moved the getTestArgs
from Settings
into Settings.Builders.RunTest
and deleted the unused cmdTestArgs
function.
src/Settings/Builders/RunTest.hs
Outdated
, pure "-dno-debug-output" | ||
] | ||
|
||
timeout_prog <- (-/- "test" -/- "bin" -/- "timeout") <$> expr buildRoot |
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.
This would be more typical: expr buildRoot <&> "test/bin/timeout"
.
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.
Fixed.
src/Settings/Builders/RunTest.hs
Outdated
|
||
let ifMinGhcVer ver opt = do v <- expr ghcCanonVersion | ||
if ver <= v then pure opt | ||
else pure "" |
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.
Isn't this simply (ver <= v) ? pure opt
?
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.
Now I move this port to src/Rules/Tests.hs
. I still use if ... then ... else
because operator ?
is only available for Expr
monad, can't be used in Action
monad.
@sighingnow This looks good to me.
Can you simply export Great job! I've left a few other minor comments above. In general, I think you sometimes could shorten the code by using the operator |
Signed-off-by: HE, Tao <[email protected]>
Now I define |
Signed-off-by: HE, Tao <[email protected]>
@sighingnow Many thanks! I think we are getting close to merging this. My apologies for slow responses, I am travelling this week. |
Signed-off-by: HE, Tao <[email protected]>
src/Builder.hs
Outdated
| Sphinx SphinxMode | ||
| Tar TarMode | ||
| Unlit | ||
| Validate |
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.
This seems to be irrelevant for this particular PR? I suggest to drop this to avoid confusion.
src/Rules/Test.hs
Outdated
] | ||
liftIO $ do | ||
writeFile (root -/- timeoutProgPath) script | ||
cmd "chmod" [ "+x", root -/- timeoutProgPath ] |
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.
There is a utility called makeExecutable
that does this.
src/Rules/Test.hs
Outdated
timeoutPyPath = "test/bin/timeout.py" | ||
|
||
timeoutProgPath :: FilePath | ||
timeoutProgPath = "test/bin/timeout" -- TODO: `.exe` suffix for windows. |
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.
You can do <.> exe
.
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.
<.> exe
will add .exe
extension to the file[1], however on linux/unix system we usually don't use specific extension for executable.
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.
It's OS-specific exactly for this reason: https://hackage.haskell.org/package/shake-0.16/docs/Development-Shake-FilePath.html#v:exe
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.
That is, on Linux "foo" <.> exe
should remain "foo"
.
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.
Oops, my bad.
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.
Done.
src/Settings/Builders/RunTest.hs
Outdated
] | ||
|
||
-- TODO | ||
, builder Validate ? pure [] ] |
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.
Not sure what should be done here, so I suggest to drop this bit.
@sighingnow I've left a few more comments. At the top of the PR one of your tasks is to add docs describing how to run tests. Are you going to add these docs as part of this PR? Or do you prefer to do it in a separate PR? Thank you! |
Signed-off-by: HE, Tao <[email protected]>
Signed-off-by: HE, Tao <[email protected]>
Signed-off-by: HE, Tao <[email protected]>
I suggest to use the Squash and merge option when merge this PR to make a clean diff for others who also is interested in this work. Thanks! |
@sighingnow Many thanks, I've squashed & merged this. Looking forward to seeing more PRs from you :) |
--test-only=<TESTS>
--skip-perf
--summary=<TEST_SUMMARY>
--junit=<TEST_SUMMARY_JUNIT>
--config=<EXTRA_TEST_CONFIG>
WIP