-
Notifications
You must be signed in to change notification settings - Fork 704
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RFC] Support for entering/leaving annotations and concurrent log linearization #6196
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
a706d8e
to
f86268c
Compare
Changes:
|
f86268c
to
19653c9
Compare
The way we currently call createProcess makes all open file descriptors leak into child processes. This is not only a cosmetic problem but also lead to really nasty concurrency bugs. This doesn't seem to really be a problem currently but I'm about to commit some code that breaks with leaking handles. Unfortunately we cannot simply use 'close_fds' always as, according to the docs, it doesn't work on windows when the std streams are not inherited from the parent process. I also introduce a separate field which allows making the handles we pass to the child process "leak" in the parent. By default 'createProcess' closes handles passed to the child process but in my case the handles are a pipe/pty-slave that I cannot easily re-open.
19653c9
to
eedb6b2
Compare
This adds support for simultaniously buffering log output in log files and "tail -f"ing the the first package in build order which is still in the process of being built. This results in build output wich is strictly ordered and exactly the same as what -j1 would produce but the actual build is run concurrently and build output shows up on the user's console live, but with only one unit's output being live at any time. That's the tradeoff. You get live output with reproducible order but it doesn't feel qute as "fast" because we're not inteleaving the build output. Initial idea from https://apenwarr.ca/log/20181106.
Ok, so here's the deal: with v2-build we can have multiple different package directories in one build. At the moment we always start GHC in the package directory with the paths to the sources being relative to that. GHC's error messages are going to copy those paths verbatim. Now when we have multiple packages in seperate directories, say: proj/ pkg-a/A.hs pkg-b/B.hs then error messages are juts going to mention "A.hs" or "B.hs" without the pkg-* prefix. So while this is kinda confusing for users that's not really the main problem. Editors (Emacs in my case) usually have a mode that parses compiler output to provide jump-to-error functionality. This usually relies on the paths in error messages being relative to the current directory of the editor or some such but we break this assumption with v2-build. It turns out we're not the first build-tool to have this problem, recursive make pretty much has the same problem and the "solution" there is to just print messages before and after starting a recursive instance of the build system in another directory. Editors already have support to parse these annotations so I'm just adding support to do that to cabal. Cabal's equivalent of the recursive make instance is Setup.hs/SetupWrapper which for v2 is always invoked through either 'buildInplaceUnpackedPackage' or 'buildAndInstallUnpackedPackage' so we add code there to print these messages. Together with the preceeding commit adding log linearizaton we can actually guarantee that the output will make sense to editors trying to parse it since it's as if we'd run with -j1, unlike the mess 'make' makes of things when concurrent builds are active!
eedb6b2
to
4873adc
Compare
It's a pity this work hasn't been merged. I seem to remember recent tickets complaining about the lack of package context in the log output. |
The pr combined three changes which combined get support for build tools |
@DanielG: would you help us get this merged this time? Let me rebase to see how the tests fare. |
@mergify rebase |
❌ Pull request can't be updated with latest base branch changesMergify needs the author permission to update the base branch of the pull request. |
thinking twice maybe a better option could be include optionally all the info in each log output, log4j style:
and let readers reorganize logs instead trying to do it when generating (so somewhat losing info) ` |
This sounds like a much larger change, no? I'd expect the format to be uniform over all/most messages, not just Entering/leaving directory... |
The main reason this can't be merged is missing windos support as can be seen from the CI failures. I could be convinced to work on just nop'ing it out there but I don't have the time or motivation to implement/test it properly there. |
Marking this PR as draft 🙂 |
Commit messages:
TODO:
--build-log
I've been using this patch set for the past couple of weeks, hence all the bug reports against 3.0/master recently and it's working really well, just the interaction with build logging on the cabal-install side needs to be figured out. I've just ignored it for now since I don't use it. Also please ignore the leading fix commits I'll submit thse for master seperately. They're just there for testing convinience of this branch.
So what do you guys think of these features?