Getting information of input test compiler #639
Conversation
There was a problem hiding this comment.
@chitrak7 Many thanks! Looks good to me in general, but I've added some comments.
| compilerPath $ testCompiler args | ||
|
|
||
| -- | Set Test Compiler | ||
| compilerPath :: String -> Action FilePath |
There was a problem hiding this comment.
I'm a bit unhappy about using strings here. Could the first argument be turned into a Stage, ideally during parsing of test arguments?
There was a problem hiding this comment.
Well, I suppose this could be turned into compilerPath :: Either Stage FilePath -> Action FilePath ?
There was a problem hiding this comment.
@alpmestan Indeed, I missed the last case.
There was a problem hiding this comment.
@alpmestan @snowleopard The input still will be a string. I will need to change command line input type as well, but I don't think we will get any major benefit from this, but needlessly complicate the code.
There was a problem hiding this comment.
@chitrak7 The benefit is in keeping parsing, string manipulation and associated errors in one place. The rest of the codebase should preferably be strongly typed.
| root -/- ghcConfigPath ~> do | ||
| ghcPath <- needfile Stage1 ghc | ||
| ghcPath <- getCompiler |
| @@ -17,6 +16,7 @@ oneZero lbl True = lbl ++ "=1" | |||
| stringToBool :: String -> Bool | |||
| stringToBool "YES" = True | |||
| stringToBool "NO" = False | |||
| stringToBool _ = error "Cannot parse string" | |||
There was a problem hiding this comment.
Let's rename this function to something more obvious, e.g. parseYesNo? Also I would prefer to make it type safe by returning Maybe Bool and handling possible failures at the use site.
Also, note that we already have something like this here:
https://github.com/snowleopard/hadrian/blob/master/src/Oracles/Flag.hs#L39-L40
As you can see, handling such errors at the use site allows for more informative error messages.
There was a problem hiding this comment.
With:
needTestBuilders :: Action ()
needTestBuilders = do
needBuilder $ Ghc CompileHs Stage2
needBuilder $ GhcPkg Update Stage1
needBuilder Hpc
needBuilder (Hsc2Hs Stage1)
timeoutProgBuilder
needTestsuitePackagesstill there, this means this patch is not done yet right? Because we likely want to need whatever the test compiler requires, no? Instead of Ghc CompileHs Stage2, GhcPkg Update Stage1, etc.
| compilerPath $ testCompiler args | ||
|
|
||
| -- | Set Test Compiler | ||
| compilerPath :: String -> Action FilePath |
There was a problem hiding this comment.
Well, I suppose this could be turned into compilerPath :: Either Stage FilePath -> Action FilePath ?
|
@chitrak7 Are you planning to complete this PR? |
|
I think we should close this PR. It has fallen behind GHC development. |
Earlier, we would get configuration from stage2 compiler irrespective of the test compiler in the input . This PR fixes this.