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

Add dump-logs (fixes #426) #2594

Merged
merged 4 commits into from
Sep 19, 2016
Merged

Add dump-logs (fixes #426) #2594

merged 4 commits into from
Sep 19, 2016

Conversation

snoyberg
Copy link
Contributor

No description provided.


-- We only want to dump logs for local non-dependency packages
case taskType of
TTLocal lp | lpWanted lp ->
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced that using lpWanted is the right thing here, we may instead want to check if this is listed as an extra-dep or not. Feedback welcome.

@harendra-kumar
Copy link
Collaborator

How about adding a note at the end of the build suggesting --dump-logs to view the logs?

@snoyberg
Copy link
Contributor Author

That could work. Logic I'd follow would be:

when (notDumpingLogs && wantedTargets > 1) displayDumpLogsSuggestion

Another idea: even when we don't dump the logs, we could display the log file locations that were generated, though for a large project even that could be overwhelming output.

@harendra-kumar
Copy link
Collaborator

Yes, it will be helpful when someone really needs it to diagnose problems. Otherwise you will have go figure the logs location first. Logs are also helpful to grep things when your console buffer has overflowed.

@snoyberg
Copy link
Contributor Author

That last comment implies to me that the full behavior should be:

when dumpLogs doFullLogDumpOutput
when (not dumpLogs && wantedTargets > 1) $ logInfo "If you want to see build output on the console, please use --dump-logs"

-- and now, regardless of the above, print the list of a log files in case the buffer overruns
logInfo "Build logs have been generated in the following locations:"
mapM_ logInfo logFilePaths

Is that what you have in mind too?

snoyberg added a commit that referenced this pull request Sep 15, 2016
This is an implementation of the discussion at:
#2594 (comment).
Now that I've seen it in practice, I'm not sure if it's too much noise
for the normal workflows. On the bright side, when dealing with just
one-package projects, none of this output ever gets displayed.
@snoyberg
Copy link
Contributor Author

I've pushed a commit with this extra output. I'm on the fence as to whether it's a good idea or not.

@harendra-kumar
Copy link
Collaborator

That sounds right to me. What are the multiple locations? Is that one for each package?

@snoyberg
Copy link
Contributor Author

It's one location per log file. There's a log file for each package's build, test, and bench output IIRC.

@harendra-kumar
Copy link
Collaborator

That might be too much addendum to the build output. Why not have a dump-logs command as well which accepts the package name argument and dumps the logs for that package. If no argument is provided it will dump all. That way we need to suggest just one command at the end and you also do not have to copy/paste the path of the log file which is usually long.

@snoyberg
Copy link
Contributor Author

I think that's quickly reaching the point of diminishing returns. How about simply saying "log files can be found in directory"?

@snoyberg
Copy link
Contributor Author

I've pushed one more commit that just have the log directory on the output. I think this is a decent middle ground.

@harendra-kumar
Copy link
Collaborator

That should work.

@borsboom
Copy link
Contributor

This looks good to me. Looks like it needs a rebase/merge from master.

This is an implementation of the discussion at:
#2594 (comment).
Now that I've seen it in practice, I'm not sure if it's too much noise
for the normal workflows. On the bright side, when dealing with just
one-package projects, none of this output ever gets displayed.
@snoyberg
Copy link
Contributor Author

Rebased against master, only conflict was a trivial one in the ChangeLog.md.

@borsboom borsboom merged commit d6b4a15 into master Sep 19, 2016
@borsboom borsboom deleted the 426-dump-logs branch September 19, 2016 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants