cabal icon indicating copy to clipboard operation
cabal copied to clipboard

[RFC] Support for entering/leaving annotations and concurrent log linearization

Open DanielG opened this issue 5 years ago • 9 comments

Commit messages:

Add entering/leaving directory messages to build output

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!
cabal-install: Add log linearization support

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.
SetupWrapper: Allow controlling FDs leaks in child procsesses

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.

  • [x] Patches conform to the coding conventions.
  • [ ] Any changes that could be relevant to users have been recorded in the changelog.
  • [ ] The documentation has been updated, if necessary.

TODO:

  • [ ] Add runtime flags controlling log-lineraization and entering-leaving messages
  • [ ] Either find a way to make it work on windows (help needed) or ifdef it away there.
  • [ ] Figure out how linearization should interact with --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?

DanielG avatar Aug 17 '19 14:08 DanielG

I should probably also mention that this will also make life easier for Haskell-IDE-Engine because we also have to (eventually) parse cabal build output for diagnostics there.

DanielG avatar Aug 20 '19 20:08 DanielG

Changes:

  • Rebase on master

DanielG avatar Mar 25 '20 14:03 DanielG

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.

ulysses4ever avatar Aug 12 '22 00:08 ulysses4ever

The pr combined three changes which combined get support for build tools I think that could be the cause of not being merged (lack of docs and tests would other ones) I think the commit logging entering and leaving directories could be merged standalone

jneira avatar Aug 12 '22 10:08 jneira

@DanielG: would you help us get this merged this time?

Let me rebase to see how the tests fare.

Mikolaj avatar Aug 12 '22 14:08 Mikolaj

@mergify rebase

Mikolaj avatar Aug 12 '22 14:08 Mikolaj

rebase

❌ Pull request can't be updated with latest base branch changes

Mergify needs the author permission to update the base branch of the pull request. @DanielG needs to authorize modification on its head branch. err-code: 38ED1

mergify[bot] avatar Aug 12 '22 14:08 mergify[bot]

I think the commit logging entering and leaving directories could be merged standalone

thinking twice maybe a better option could be include optionally all the info in each log output, log4j style:

[${timestamp}] ${logLevel} [${theadId}] [${package name}] actual log msg

and let readers reorganize logs instead trying to do it when generating (so somewhat losing info)

`

jneira avatar Aug 13 '22 15:08 jneira

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...

ulysses4ever avatar Aug 13 '22 17:08 ulysses4ever

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.

DanielG avatar Aug 22 '22 19:08 DanielG

Marking this PR as draft :slightly_smiling_face:

Kleidukos avatar May 17 '23 06:05 Kleidukos