Remove validation errors #628

chitrak7 · 2018-06-17T17:32:14Z

I have copied some files as required by testsuite.

Also, I have unsuccessfully attempted to resolve #626.

snowleopard · 2018-06-18T10:09:13Z

src/GHC.hs

@@ -107,9 +107,10 @@ stage2Packages = return [haddock]
 testsuitePackages :: Action [Package]
 testsuitePackages = return [ checkApiAnnotations


We have this list of packages needed for the testsuite, but then do a lot of ad hoc needs in the testsuite instead of fully relying on this list. Can we do better?

Actually, default libraries are built vanilla way, but the libraries listed here are needed in profiling and dynamic ways also. Can we create a separate list of packages needed only for testsuite and have to built in profiling and dynamic ways.

So, maybe this list should list ways too?

I think someone needs to think about the overall architecture of the testsuite. So far there are a bunch of PRs that seem to introduce various ad hoc solutions, but I do not see a clear design of how everything should be tied together. Can you write up such a design document?

Yes, I will be creating an Issue in this regards soon enough.

Yes we need to be a lot more careful about those uses of vanillaContext and default ways for packages etc. Most of the time we just want to follow what libraryWays and rtsWays dictate, I think. I'm expecting this single line of work to fix a lot of test failures.

snowleopard · 2018-06-18T10:10:25Z

src/Rules/Test.hs

+    binPath        <- stageBinPath Stage1
+    libPath        <- stageLibPath Stage1
+    iservPath      <- needfile Stage1 iserv 
+    runhaskellPath <- needfile Stage1 runGhc


Shall iserv and runGhc be added to testsuitePackages?

Yes, It we need iserv in testsuite packages as we need to build its profiling way.
runGhc is already in stage1 and I dont't think we need to build any other way here. But we can add it just to be sure. What are your suggestions @snowleopard .

The testsuitePackages should contain all packages required for the testsuite. That's what the name suggests!

Aha, I see where the confusion can be coming from. The comment says: "Packages that are built only for the testsuite", but I think it should actually say "Packages that are required for the testsuite".

Resolved. There are still few questions regarding this issue but I think I should create a separate issue for that.

snowleopard · 2018-06-18T10:11:19Z

src/Rules/Test.hs


        -- Set environment variables for test's Makefile.
        liftIO $ do
            setEnv "MAKE" makePath
            setEnv "TEST_HC" ghcPath
            setEnv "TEST_HC_OPTS" ghcFlags
+            setEnv "CHECK_PPR" checkPprPath
+            setEnv "CHECK_API_ANNOTATIONS" annotationsPath 

        -- Execute the test target.
        buildWithCmdOptions env $ target (vanillaContext Stage2 compiler) RunTest [] []

 -- | Build extra programs required by testsuite


I've already commented on this: the name needTestsuiteBuilders is misleading -- this function deals not only with "builders" but also with libraries. It should therefore be renamed, perhaps to needTestsuitePackages.

snowleopard · 2018-06-18T10:12:15Z

src/Rules/Test.hs

-      -- we are going to use, I suppose?
-      | isLibrary pkg = pkgConfFile (vanillaContext stage pkg)
-      | otherwise = programPath =<< programContext stage pkg
+    copyFile iservPath $ libPath -/- "bin/ghc-iserv"


Why do we need to do the copying? We should add a comment here to clarify the logic.

Yes, updating in a revision.

snowleopard · 2018-06-18T10:13:15Z

@chitrak7 Thank you! Please see my comments above.

snowleopard · 2018-06-18T10:50:41Z

src/Settings/Builders/GhcCabal.hs

@@ -57,21 +57,25 @@ ghcCabalBuilderArgs = mconcat
 -- TODO: Isn't vanilla always built? If yes, some conditions are redundant.
 -- TODO: Need compiler_stage1_CONFIGURE_OPTS += --disable-library-for-ghci?
 -- TODO: should `elem` be `wayUnit`?
+-- This approach still doesn't work. Previously libraries were build only in the 
+-- Default flavours and not using context. 


This might actually be dead code, since we no longer call ghc-cabal. Are you sure this function is actually executed?

Oh, maybe this is why this approach is not working. Can you please tell me how exactly we are configuring cabal files then!

@chitrak7 I'll try to find time today to remove dead code. You should look into the module Hadrian.Haskell.Cabal.Parse.

chitrak7 · 2018-06-18T15:58:53Z

@snowleopard does ghc always make vanilla?

snowleopard · 2018-06-18T16:12:14Z

does ghc always make vanilla?

@chitrak7 Yes, I think it does.

chitrak7 · 2018-06-18T17:14:11Z

@snowleopard I find the logic testsuite uses to find which ways libraries are built quite weak. It simply checks for the presence/absence of PrimoWrappers.hi/dyn_hi/p_hi. Should I use the same logic also??
Is there any other way to do this?

snowleopard · 2018-06-18T17:29:46Z

@chitrak7 Why don't you look at the contents of ways?

alpmestan · 2018-06-18T18:03:23Z

Ideally I think we really want to examine the libraryWays and rtsWays of the Flavour we are running with, because it is supposed to contain precisely those pieces of information as far as my understanding goes. If you find that for some reason this is not reliable enough (or falls short in some cases), well first please let me know because I'm curious about this, but we can then simply go with the slightly more "hacky" strategy of checking the existence of build products with all those way-specific extension to figure out what we have actually built. Sounds good to everyone?

snowleopard · 2018-06-18T18:16:00Z

@alpmestan As I mentioned elsewhere, I think we might want to add a new build flavour, specifically for the testsuite, listing precisely all the ways that we'd like to test (and therefore build).

In other words, the build for testing is different from a usual (default) build: not only we build a different collection of packages, we might also want to specify precisely which ways we want.

chitrak7 · 2018-06-18T18:57:24Z

@snowleopard I have added fore features to this PR.

Why don't you look at the contents of ways?

Can you please explain what you mean when you say this. Exactly whose ways are we supposed to look?

alpmestan · 2018-06-18T22:03:52Z

@alpmestan As I mentioned elsewhere, I think we might want to add a new build flavour, specifically for the testsuite, listing precisely all the ways that we'd like to test (and therefore build).

I'm not sure I agree with this. The behaviour of the Make build system here, when you e.g say make slowtest, is to run all the tests that are not skipped given the configuration/build artifacts available. So if you don't have profiling libs, all tests that require them will be skipped (which is different from failing, they're just not considered for execution, like Windows tests are not considered for execution on Linux). I would therefore have thought that the --flavour could dictate what we produce and that the testsuite could just go with that, skipping all the tests that require other things.

This by the way does not exclude having a dedicated flavour that just has everything the testsuite needs to run all the tests on a given system.

chitrak7 · 2018-06-19T06:18:54Z

@snowleopard I think I agree with Alp on this one. We should instead modify Hadrian to read input test ways and build libraries based on them. This is what I was trying to fix through when I encountered #626. Aside from that, I favor using input flavors and build flavors to build libraries instead of checking the libraries we have already installed.

chitrak7 · 2018-06-19T09:40:57Z

@snowleopard I think I am finished with major part of this issue. I will handle library ways in a later PR. Can you review this one at present.

alpmestan

Minor comment, but this looks good.

alpmestan · 2018-06-19T10:45:11Z

src/GHC.hs

@@ -107,9 +107,10 @@ stage2Packages = return [haddock]
 testsuitePackages :: Action [Package]
 testsuitePackages = return [ checkApiAnnotations


Yes we need to be a lot more careful about those uses of vanillaContext and default ways for packages etc. Most of the time we just want to follow what libraryWays and rtsWays dictate, I think. I'm expecting this single line of work to fix a lot of test failures.

alpmestan · 2018-06-19T10:50:10Z

src/Settings/Builders/RunTest.hs

+    withNativeCodeGen <- expr $ fmap stringToBool (testSetting TestGhcWithNativeCodeGen)
+    withInterpreter   <- expr $ fmap stringToBool (testSetting TestGhcWithInterpreter)
+    unregisterised    <- expr $ fmap stringToBool (testSetting TestGhcUnregisterised)
+    withSMP           <- expr $ fmap stringToBool (testSetting TestGhcWithSMP)


I think it's safe to say that we might want a shorter name for expr $ fmap stringToBool (testSetting ...) (or just the fmap stringToBool (testSetting ...) part?) =)

We will need it for properly configuring python command Some revisions

sighingnow

LGTM. With two minor comments.

sighingnow · 2018-06-19T11:46:07Z

src/Rules/Test.hs

+        ghcPath         <- needfile Stage1 ghc
+        need [ root -/- ghcConfigProgPath]
+        command [FileStdout $ root -/- ghcConfigPath] (root -/- ghcConfigProgPath)
+            [ ghcPath  ] 


I think the variable ghcPath may be incorrect. In ghc's testsuite/mk/boilerplate.mk we pass the $(TEST_HC) as argument to ghc-config. However in the "test" rule, we use stage2 compiler as TEST_HC. I think here the consistency does matter.

Ref: https://github.com/ghc/ghc/blob/master/testsuite/mk/boilerplate.mk#L240

Here also, the stage2 compiler is used only. We just build stage2 compiler in stage1 and this is why I have used stage1.

In need needTestBuilders we ensure the stage2 ghc is ready before run test. The stage1 and stage2 compiler are often built in different ways (for example, with or without -prof). We are testing again the stage2 compiler, I think it does matter to use stage2 compiler to get these test settings (for example, GhcWithInterpreter, GhcProfiled and GhcDebugged).

We just build stage2 compiler in stage1 and this is why I have used stage1.

I think we could needBuilder $ Ghc CompileHs Stage2 in rule root -/- ghcConfigPath, before executing the ghc-config command.

@sighingnow when I call needfile Stage1 ghc, I call the path of stage2 compiler only. Now, stage2 compiler is built in _build/stage1/bin/ghc. Hence, I am still pointing to stage2 compiler only.

Yes you are correct. Thanks for the explanation. I just found that hadrain places the stage2 ghc inside the stage1 folder....

Now this PR looks good to me.

Thank you for the review @sighingnow!

h I just found that hadrain places the stage2 ghc inside the stage1 folder....

Right, because it is built by stage1. It is however indeed confusing at first, but it makes sense to me now.

sighingnow · 2018-06-19T11:52:28Z

src/Oracles/TestSettings.hs

+testRTSSettings :: Action [String]
+testRTSSettings = do 
+    file <- testConfigFile
+    fmap words $ lookupValueOrError file "GhcRTSWays"


I think the testRTSSettings is just a case of testSetting.

How about add a TestGhcRTSWays constructor to TestSetting and move the lookupValueOrError file "GhcRTSWays" part into testSetting, then the testRTSSettings will be:

testRTSSettings :: Action [String] testRTSSettings = words <$> testSetting TestGhcRTSWays

More concise.

@sighingnow I just refrained from using RTSSetting with testSetting because someone might use testSetting instead of testRTSSetting and get errors. I wanted to separate RTSWays completely from testSetting so as no confusion is created.

I see. Thanks for making this point clear to me.

snowleopard · 2018-06-19T13:49:32Z

@alpmestan @chitrak7 OK, you convinced me :)

Then, what about producing a file, e.g. _build/flavour, after each successful build that essentially provides all necessary information for the testsuite (or other possible consumers in the toolchain)? This will make it possible for the user to run Hadrian using their custom build flavour and then run the testsuite, which will pick up the correct build configuration without relying on hacks like checking for p_dyn objects? This will also not tie the testsuite only to the default build flavour.

snowleopard · 2018-06-19T13:51:27Z

An alternative is then, which you seem to suggest, is to require the user to run the testsuite with exactly the same flavour as during the build.

For example, build --flavour=user must be followed by build test --flavour=user. This might work too.

snowleopard · 2018-06-19T13:52:10Z

src/Oracles/TestSettings.hs

+                 | TestMinGhcVersion801
+                 deriving (Show)
+
+-- | Lookup for testsettings in ghcconfig file


Please provide the full path to the ghcconfig file.

@snowleopard Why do we need the full path. The provided path is relative to buildroot.

I'm referring to the comment. You should keep in mind the readers that have no idea what this code is supposed to do. The full path will be helpful for them.

snowleopard · 2018-06-19T13:54:54Z

src/Rules/Test.hs

+    -- | Using program shipped with testsuite to generate ghcconfig file.
+    root -/- ghcConfigProgPath ~> do
+        ghc             <- builderPath $ Ghc CompileHs Stage0
+        command [] ghc [ghcConfigHsPath, "-o" , root -/- ghcConfigProgPath]


Could you switch to using cmd instead of command as in the rest of Hadrian?

snowleopard · 2018-06-19T13:56:27Z

src/Rules/Test.hs

-    targets <- mapM (needfile Stage1) =<< testsuitePackages
+-- | Build extra programs and libraries required by testsuite
+needTestsuiteLibraries :: Action ()
+needTestsuiteLibraries = do


But needTestsuiteLibraries is as misleading as needTestsuiteBuilders!

Why not needTestsuitePackages?

snowleopard · 2018-06-19T13:59:04Z

src/Oracles/TestSettings.hs

+import Hadrian.Oracles.TextFile
+import Base
+
+testConfigFile :: Action FilePath


It would be great to add more comments here about the purpose of this module.

Please describe the purpose of the file test/ghcconfig, give a couple of examples of the settings that you read from it, and how they are used by the testsuite.

snowleopard · 2018-06-19T14:01:08Z

@chitrak7 I've added a couple of more comments. Please have a look.

alpmestan · 2018-06-19T14:09:04Z

An alternative is then, which you seem to suggest, is to require the user to run the testsuite with exactly the same flavour as during the build.

For example, build --flavour=user must be followed by build test --flavour=user. This might work too.

Right. Or really you'd just do build test --flavour=something (where something is an existing or user defined flavour). This is a quite common need. Say you investigate some ticket, write a patch and want to see if that fixes the failure of test foo. You can then just go ahead and do build test --flavour=suitable_flavour --build-root=_test-foo --only=foo and have a cup of tea/coffee, where suitable_flavour should be whatever is appropriate: quickest if the test doesn't need any fancy way, devel2/perf/prof/etc depending on what's needed.

One reason why I think this works rather nicely is that this is the most flexible approach. I see two alternatives. The first one, at the opposite end of the spectrum, is to just require all the ways of everything to be built before we can run any test. This is annoying as it will significantly increase the duration of the build. The other one, sitting in the middle, is only conceptual but is the best one. It'd consist in building just what we need for just those tests the user is asking to run (as determined by command line options etc). However the testsuite doesn't really give us a way to discover this ahead of time, as far as I know. I mean, we could scan the .T files and try to inspect the source code to figure some things out but... I don't think we want to do this :-) So I think it's a good choice to let users specify a flavour that's compatible with the tests they want to run.

Now... in this scheme that I'm suggesting, what if we call the test rule again on the same build root but with a different flavour than the one we called it on right before? I don't think this would rebuild things with the way our library/executable rules are implemented at the moment. It feels like we should rebuild everything so as to actually produce all the build artefacts variants that the new flavour implies. A bit like how cabal-install does it. Thoughts?

snowleopard · 2018-06-19T14:16:05Z

@alpmestan Yes, I agree, this looks like the right approach indeed.

Now... in this scheme that I'm suggesting, what if we call the test rule again on the same build root but with a different flavour than the one we called it on right before? I don't think this would rebuild things with the way our library/executable rules are implemented at the moment.

Hmm, are you sure the rebuilds won't happen? We track all command line arguments, so if changing the flavour means a command line changes (e.g. you add -prof) then all affected targets will be rebuilt.

sighingnow · 2018-06-19T14:20:02Z

what if we call the test rule again on the same build root but with a different flavour than the one we called it on right before?

I agree with this idea. make test doesn't trigger rebuild even if there are changes in sources. I think we should decouple the dependency between the stage1/2 ghc and the hadrain test command. We only need to make sure the corresponding ghc compiler exists. If not, we can report error to user and suggest the hadrian build --flavour=xxx command.

alpmestan · 2018-06-19T14:39:21Z

Hmm, are you sure the rebuilds won't happen? We track all command line arguments, so if changing the flavour means a command line changes (e.g. you add -prof) then all affected targets will be rebuilt.

@snowleopard I'm not entirely sure, and I'm in the middle of something right now so I can't really give it a shot.

I agree with this idea. make test doesn't trigger rebuild even if there are changes in sources. I think we should decouple the dependency between the stage1/2 ghc and the hadrain test command. We only need to make sure the corresponding ghc compiler exists. If not, we can report error to user and suggest the hadrian build --flavour=xxx command.

@sighingnow Right. I'll be happy as long as I can start from a fresh source tree and just call build -c test and have this command build a GHC. Adding --flavour should then refine what gets built and possibly rebuild things, and the test rules should just react accordingly, picking up or dropping tests depending on what build products are available.

Note that this is the default behaviour of make test indeed but not of the validate script, which by default uses a pre-determined .mk file but lets you supply your own. It might be interesting to map this pre-determined build specification to a Flavour in hadrian land and use it for the validate rule (and just that one, not the test rule) when no --flavour is specified? We would therefore have build validate ~ ./validate and build test ~ make [slow|fast|full]test. And supply a --flavour explicitly would amount to writing a custom validate.mk to be picked up by the ./validate script.

chitrak7 · 2018-06-19T15:26:34Z

src/Settings/Builders/Ghc.hs

@@ -79,6 +79,7 @@ commonGhcArgs = do
            , arg "-hcsuf", arg $ hcsuf way
            , wayGhcArgs
            , packageGhcArgs
+            , arg "-lnuma"


@snowleopard I incurred an error which I think is due to #559 and added an additional flag to resolve this. I accidentally committed the change but think might be helpful. Should I remove the argument?

We shouldn't add such flags unconditionally. Better do this in a separate PR.

alpmestan · 2018-06-19T15:28:09Z

src/Settings/Builders/Ghc.hs

@@ -79,6 +79,7 @@ commonGhcArgs = do
            , arg "-hcsuf", arg $ hcsuf way
            , wayGhcArgs
            , packageGhcArgs
+            , arg "-lnuma"


Shouldn't this be dependent on whether we enable NUMA support in ./configure ?

I supplied no such flag, neither is there any such flag with configure script help. I think it is default setting. Reverting this change and looking further into the matter.

snowleopard · 2018-06-19T16:57:53Z

OK, looks like all comments have been addressed. Shall I merge this?

chitrak7 · 2018-06-20T12:09:31Z

@alpmestan @snowleopard Is this pull ready to land.

snowleopard · 2018-06-20T12:15:06Z

@chitrak7 Merged, thanks!

snowleopard reviewed Jun 18, 2018

View reviewed changes

chitrak7 force-pushed the testing branch 2 times, most recently from 931ab04 to a56333e Compare June 18, 2018 10:37

snowleopard reviewed Jun 18, 2018

View reviewed changes

chitrak7 closed this Jun 19, 2018

chitrak7 reopened this Jun 19, 2018

chitrak7 changed the title ~~[WIP] Remove validation errors~~ Remove validation errors Jun 19, 2018

alpmestan previously approved these changes Jun 19, 2018

View reviewed changes

chitrak7 added 6 commits June 19, 2018 16:52

Improved GhcCabal library arguments

1f83ec8

setting enviornment for extra programs

62253e1

Copied necessary programs

22077cc

Added support to generate ghcconfig

977d3d6

We will need it for properly configuring python command Some revisions

Using ghcconfig file for test parameters

bd6ab61

minor changes

29ea70b

chitrak7 dismissed alpmestan’s stale review via 29ea70b June 19, 2018 11:42

chitrak7 force-pushed the testing branch from b3b3ab6 to 29ea70b Compare June 19, 2018 11:42

sighingnow previously approved these changes Jun 19, 2018

View reviewed changes

snowleopard reviewed Jun 19, 2018

View reviewed changes

minor revision

fe12b50

chitrak7 dismissed sighingnow’s stale review via fe12b50 June 19, 2018 15:14

chitrak7 commented Jun 19, 2018

View reviewed changes

alpmestan reviewed Jun 19, 2018

View reviewed changes

Update Ghc.hs

7c9f683

snowleopard merged commit d4b9c1f into snowleopard:master Jun 20, 2018

chitrak7 deleted the testing branch June 20, 2018 17:21

chitrak7 mentioned this pull request Jun 20, 2018

Build Profiling and Dynamic Ways for libraries #626

Closed

		@@ -107,9 +107,10 @@ stage2Packages = return [haddock]
		testsuitePackages :: Action [Package]
		testsuitePackages = return [ checkApiAnnotations

Remove validation errors #628

Remove validation errors #628

Conversation

chitrak7 commented Jun 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowleopard commented Jun 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chitrak7 commented Jun 18, 2018

snowleopard commented Jun 18, 2018

chitrak7 commented Jun 18, 2018

snowleopard commented Jun 18, 2018

alpmestan commented Jun 18, 2018 • edited Loading

snowleopard commented Jun 18, 2018

chitrak7 commented Jun 18, 2018

alpmestan commented Jun 18, 2018 • edited Loading

chitrak7 commented Jun 19, 2018 • edited Loading

chitrak7 commented Jun 19, 2018

alpmestan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sighingnow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowleopard commented Jun 19, 2018

snowleopard commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowleopard commented Jun 19, 2018

alpmestan commented Jun 19, 2018 • edited Loading

snowleopard commented Jun 19, 2018

sighingnow commented Jun 19, 2018 • edited Loading

alpmestan commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowleopard commented Jun 19, 2018

chitrak7 commented Jun 20, 2018

snowleopard commented Jun 20, 2018

chitrak7 commented Jun 17, 2018 •

edited

Loading

alpmestan commented Jun 18, 2018 •

edited

Loading

alpmestan commented Jun 18, 2018 •

edited

Loading

chitrak7 commented Jun 19, 2018 •

edited

Loading

alpmestan commented Jun 19, 2018 •

edited

Loading

sighingnow commented Jun 19, 2018 •

edited

Loading