Skip to content
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

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DanielG
Copy link
Collaborator

@DanielG DanielG commented Aug 17, 2019

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.

  • 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

This comment has been minimized.

@DanielG DanielG force-pushed the log-linearization branch from a706d8e to f86268c Compare March 25, 2020 14:40
@DanielG
Copy link
Collaborator Author

DanielG commented Mar 25, 2020

Changes:

  • Rebase on master

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.
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!
@ulysses4ever
Copy link
Collaborator

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.

@jneira
Copy link
Member

jneira commented Aug 12, 2022

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

@Mikolaj
Copy link
Member

Mikolaj commented Aug 12, 2022

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

Let me rebase to see how the tests fare.

@Mikolaj
Copy link
Member

Mikolaj commented Aug 12, 2022

@mergify rebase

@mergify
Copy link
Contributor

mergify bot commented Aug 12, 2022

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

@jneira
Copy link
Member

jneira commented Aug 13, 2022

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)

`

@ulysses4ever
Copy link
Collaborator

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

@DanielG
Copy link
Collaborator Author

DanielG commented Aug 22, 2022

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.

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 1, 2022
@ulysses4ever ulysses4ever removed the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Sep 3, 2022
@Kleidukos
Copy link
Member

Marking this PR as draft 🙂

@Kleidukos Kleidukos marked this pull request as draft May 17, 2023 06:34
@ulysses4ever ulysses4ever added attention: needs-help Help wanted with this issue/PR re: user experience User experience (UX) issue attention: needs-rebase labels Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention: needs-help Help wanted with this issue/PR attention: needs-rebase re: user experience User experience (UX) issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants