Accept tests megapatch #4337
Accept tests megapatch #4337
Conversation
|
Do the patches make sense individually? If so, I'll try to at least do a brief review today. |
|
Yep. Some of them are "refactor in preparation for" but they should all be self-contained. |
| -- Polymorphic to take | ||
| -- 'OpenModule' or 'Module' | ||
| -> IO () | ||
| setupMessage' verbosity msg pkgid cname mb_insts = withFrozenCallStack $ do |
There was a problem hiding this comment.
The "prime" naming convention is from your other work on taking a Verbosity flag, right?
There was a problem hiding this comment.
It's not really much of a naming convention. In this case, the prime means, "and takes more arguments".
Cabal/Distribution/Simple/Haddock.hs
Outdated
| @@ -209,24 +210,29 @@ haddock pkg_descr lbi suffixes flags' = do | |||
| warn (fromFlag $ haddockVerbosity flags) | |||
| "Unsupported component, skipping..." | |||
| return () | |||
| -- Don't print this message if nothing is going to happen... | |||
There was a problem hiding this comment.
Not sure the comment makes sense
There was a problem hiding this comment.
It's saying that we shouldn't print this message if no Haddocking will occur. Previously, the setup message was unconditionally printed. Now we put the action in smsg and call it inside the cases.
|
|
||
| -- | The 'Progress' monad with specialized logging and | ||
| -- error messages. | ||
| newtype LogProgress a = LogProgress (Progress LogMsg Doc a) | ||
| deriving (Functor, Applicative, Monad) | ||
| newtype LogProgress a = LogProgress { unLogProgress :: LogEnv -> Progress LogMsg Doc a } |
There was a problem hiding this comment.
Not a big deal, but isn't there a Reader(T) somewhere from a library that Cabal already depends on?
There was a problem hiding this comment.
Cabal doesn't depend on transformers (yet). When parsec becomes default it will but I don't think that GHC's build system has been adjusted to handle this yet.
| @@ -63,11 +63,15 @@ runLogProgress verbosity (LogProgress m) = | |||
|
|
|||
| -- | Output a warning trace message in 'LogProgress'. | |||
| warnProgress :: Doc -> LogProgress () | |||
| warnProgress s = LogProgress $ \_ -> stepProgress (LogMsg normal (text "Warning:" <+> s)) | |||
| warnProgress s = LogProgress $ \env -> | |||
| when (le_verbosity env >= normal) $ | |||
There was a problem hiding this comment.
Heh, "le verbosity" sounds like a meme of some kind :).
| let reqs = modScopeRequires linked_shape0 | ||
| -- check that there aren't pre-filled requirements... | ||
| let reqs = Map.keysSet (modScopeRequires linked_shape0) | ||
| `Set.union` Set.fromList src_reqs |
There was a problem hiding this comment.
Is the union here an effective change in behavior? If so, maybe it should be mention in commit msg.
There was a problem hiding this comment.
I guess it is, but for a reason more subtle than reqs including the 'sigantures' field (in the old code, it was mixed in). I'll mention it.
| Left err -> return (Left err) | ||
| Right x'' -> return (Right (f'' x'')) | ||
| Nothing -> return Nothing | ||
| Just x'' -> return (Just (f'' x'')) |
There was a problem hiding this comment.
Isn't this case just a liftM/fmap?
There was a problem hiding this comment.
Unfortunately not, because it's IO (Maybe a) so an fmap will use the IO instance, not the MaybeT IO instance. But we can't depend on MaybeT, so it's done by hand...
| Left err -> return (Left err) | ||
| Right x' -> unUnifyM (f x') r | ||
| Nothing -> return Nothing | ||
| Just x' -> unUnifyM (f x') r |
| env <- getUnifEnv | ||
| -- TODO: Perhaps it would be wise to defer running these | ||
| -- actions until the very end? | ||
| -- ctx <- sequence (unify_ctx env) |
There was a problem hiding this comment.
The comment seems to contradict the fact that the next line is commented out.
There was a problem hiding this comment.
Yes, this comment is stale, I'll remove it.
| Just _i -> error "convertUnitIdU': mutual recursion" -- return (UnitIdVar i) | ||
| Just _i -> | ||
| failWith (text "Unsupported mutually recursive unit identifier") | ||
| -- return (UnitIdVar i) |
There was a problem hiding this comment.
Remove the commented bit?
There was a problem hiding this comment.
Well, it's what you would replace the line with if you added support for mutual recursion :)
| req_rename_list <- | ||
| case req_rns of | ||
| DefaultRenaming -> return [] | ||
| -- Not valid here, but whatever |
There was a problem hiding this comment.
Move comment to next line just to make it absolutely clear which case it applies to?
ezyang
force-pushed
the
pr/accept-tests-megapatch
branch
from
13a62df to
8357a43
Compare
|
Repushed with updates for comments. |
Signed-off-by: Edward Z. Yang <[email protected]>
I noticed that I was repeatedly writing the same code
to print out more elaborate information when we do builds,
so I refactored it all into one place. In the process,
I think that I have made the build output more generally
useful.
The key changes:
- There is a new function setupMessage' which takes in
more information than the conventional setupMessage
does, and prints a more informative message: whereas
setupMessage will only tell you about the package
it is being run in, setupMessage' will also tell
you about the component and instantiation.
- I applied this function to applicable sites, in some
cases moving around messages to be closer to the place
where an actual operation takes place. For example,
the 'Building' message previously only was triggered
at the beginning of the build process; now it is
emitted immediately before we call out to GHC. This
is a lot more informative, and avoids people thinking
that we are slow because of preprocessing (we're not.)
Something similar happened for Haddock as well.
Before:
Preprocessing library 'spider' for reflex-backpack-0.5.0..
[1 of 1] Compiling Reflex.Spider.Backpack ( src/Reflex/Spider/Backpack.hs, /srv/code/reflex-backpack/dist-newstyle/build/x86_64-linux/ghc-8.1.20170123/reflex-backpack-0.5.0/c/spider/build/spider/Reflex/Spider/Backpack.o )
After:
Preprocessing library 'host' for reflex-backpack-0.5.0..
Building library 'host' instantiated with
Reflex.Host.Sig = reflex-backpack-0.5.0-inplace-spider:Reflex.Spider.Backpack
Reflex.Sig = reflex-backpack-0.5.0-inplace-spider:Reflex.Spider.Backpack
for reflex-backpack-0.5.0..
[1 of 8] Compiling Reflex.Host.Sig[sig] ( host/Reflex/Host/Sig.hsig, /srv/code/reflex-backpack/dist-newstyle/build/x86_64-linux/ghc-8.1.20170123/reflex-backpack-0.5.0/c/host/reflex-backpack-0.5.0-inplace-host+FDoWUmUc0MMBtBRwItgjj9/build/reflex-backpack-0.5.0-inplace-host+FDoWUmUc0MMBtBRwItgjj9/Reflex/Host/Sig.o ) [Reflex.Basics changed]
Signed-off-by: Edward Z. Yang <[email protected]>
Signed-off-by: Edward Z. Yang <[email protected]>
Signed-off-by: Edward Z. Yang <[email protected]>
Signed-off-by: Edward Z. Yang <[email protected]>
New output:
Error:
Problem with module re-exports:
- The module 'Asdf'
is not exported by any suitable package.
It occurs in neither the 'exposed-modules' of this package,
nor any of its 'build-depends' dependencies.
In the stanza 'library'
In the inplace package 'Reexport2-1.0'
Signed-off-by: Edward Z. Yang <[email protected]>
The semantic change in this diff has to do with how local signatures are treated. Previously, we threw them into the bag when mix-in linking, which means that if you declared a signature, and another package brought that module into scope, the signature would get "filled". This is actually never what you want in non-recursive Backpack, so this commit moves it so that we only put the signatures in post-facto. There is still some more sanity checking we should do but I'm deferring that for a later commit. Signed-off-by: Edward Z. Yang <[email protected]>
Signed-off-by: Edward Z. Yang <[email protected]>
Signed-off-by: Edward Z. Yang <[email protected]>
Signed-off-by: Edward Z. Yang <[email protected]>
Signed-off-by: Edward Z. Yang <[email protected]>
Now if you say cabal -v"info +callstacks", Cabal invocations will also get call stacks. There's a heinous hack to handle version of Cabal that don't support the extended format. Signed-off-by: Edward Z. Yang <[email protected]>
Signed-off-by: Edward Z. Yang <[email protected]>
Signed-off-by: Edward Z. Yang <[email protected]>
Signed-off-by: Edward Z. Yang <[email protected]>
Signed-off-by: Edward Z. Yang <[email protected]>
Signed-off-by: Edward Z. Yang <[email protected]>
Signed-off-by: Edward Z. Yang <[email protected]>
Signed-off-by: Edward Z. Yang <[email protected]>
Signed-off-by: Edward Z. Yang <[email protected]>
Signed-off-by: Edward Z. Yang <[email protected]>
Signed-off-by: Edward Z. Yang <[email protected]>
Signed-off-by: Edward Z. Yang <[email protected]>
This flips error handling around, so that 'die' now can format an error message with call stacks and markers before raising it to the top handler. The top handler detects "verbatim" deaths and prints them without formatting. Signed-off-by: Edward Z. Yang <[email protected]>
Signed-off-by: Edward Z. Yang <[email protected]>
In general, the test suite will not have any servers configured so this warning is not very useful. Signed-off-by: Edward Z. Yang <[email protected]>
Signed-off-by: Edward Z. Yang <[email protected]>
- We switched to using regex-tdfa, as regex-posix is buggy on Windows. Signed-off-by: Edward Z. Yang <[email protected]>
Signed-off-by: Edward Z. Yang <[email protected]>
ezyang
force-pushed
the
pr/accept-tests-megapatch
branch
from
8357a43 to
42e0d4f
Compare
Signed-off-by: Edward Z. Yang <[email protected]>
Signed-off-by: Edward Z. Yang <[email protected]>
ezyang
force-pushed
the
pr/accept-tests-megapatch
branch
from
4d631f9 to
34be45f
Compare
|
Because this commit is so huge, I'm going to go ahead and merge it. But please feel free to add more comments, I will address them ASAP. |
|
OK, sure. I'll try to take a look today and merge it into 2.0. |
|
Oh, I see you already did it. Good. |
If possible, I'd like this patchset to go on the 2.0 branch.
die'function which takes Verbosity, and can emit callstacks. This patch touched a lot of code (which is why I want it in 2.0)