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

Write debug output to stderr #7838

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

Conversation

fredefox
Copy link
Contributor

This fixes #7790


Please include the following checklist in your PR:

I tested this by running cabal run against another projects, verifying that all cabal-related output went into stderr, and output from my application went to stdout.

@fredefox
Copy link
Contributor Author

I struggle a bit to parse the output of the test failure. If anyone could nudge me in the right direction. That would be appreciated.

@Mikolaj
Copy link
Member

Mikolaj commented Nov 26, 2021

From what I see, it expected to find the following on stderr, but didn't see it:

Test suite LibV09-Deadlock: FAIL
Test suite logged to:
setup-deadlock.dist/work/dist/test/LibV09-0.1-LibV09-Deadlock.log

and you can find in the log earlier that this appears in stdout now, nondeterministically interleaved with some other output to stdout. Or it may all be the other way around.

@fredefox
Copy link
Contributor Author

fredefox commented Nov 26, 2021

I'm realising now that the problem is not as simple as I first thought. For a starter cabal-testsuite does not keep track of whether output was written to stdout or stderr. That would be the first thing to fix. As an added kink, cabal-testsuite writes program output to a log file in which the expectation is (of course) that the program output occurs in the order it was written. Regardless of which pipe it was written to!

If we want to retain that we'd need some sort of clever piping in Run.hs. I struggle to figure out exactly how this would look. But it should be doable.

@fredefox fredefox marked this pull request as draft November 26, 2021 22:57
@fredefox
Copy link
Contributor Author

Had a stab at it just now. See my attempts in cabal-testsuite/src/Test/Cabal/Run.hs. It's a bit messy with all those pipes. It doesn't quite work. In testing I discovered that the value of rAll appears out of order. I tested it by executing /bin/bash /tmp/test.sh with test.sh being:

echo stdout
echo stderr >&2

The value of rAll in this case was:

stderr
stdout

@Mikolaj
Copy link
Member

Mikolaj commented Nov 27, 2021

Concurrency is hard. If that helps, feel free to open a new PR exclusively about a given kind of minimal cabal-testsuite improvement. That would also make reviewing and testing easier.

@fredefox
Copy link
Contributor Author

fredefox commented Nov 27, 2021

Concurrency is hard. If that helps, feel free to open a new PR exclusively about a given kind of minimal cabal-testsuite improvement. That would also make reviewing and testing easier.

Yes. I think this is the way to go. I'll concern myself with splitting it up later though. For now I'm just hacking around. So to be clear there are currently two tasks:

  • Fix cabal-testsuite so that it recognizes which file descriptor output goes to. This may also mean significant changes to each individual test, since they currently make no such distinction. Alternatively we could consider supporting a testing mode where we don't care which file descriptor receives output.
  • Change where Cabal writes output to. Fixup tests accordingly.

@Mikolaj
Copy link
Member

Mikolaj commented Nov 27, 2021

We could also consider a testing mode where we don't care which file descriptor receives output.

Yes, I guess, that would make it possible to cheaply migrate most test and tweak only a couple to test the actual split. However, the mode would need to guarantee the combined output is deterministic. That's probably hard to do without flushing all the time.

@fredefox
Copy link
Contributor Author

fredefox commented Nov 27, 2021

I see there was previous effort to work on this #4789. I wonder if that PR is still receiving life support. Judging by the creation date I would guess it's covered in cob webs. It also looks like there was a bit of a discussion around the desired functionality. @hvr mentioned that there might be cases where you would actually want Cabal to write to stdout. I think it could be good to consolidate some of the different discussions that are happening at #7838 (here), #4789, #7790, #5595, #4652. An authoritative and slightly more in-depth answer to the question "what output should go where?" would be needed to tackle #7790 but shouldn't stand in the way of expanding the capabilities of cabal-testsuite mentioned in bullet no. 1 above.

@Mikolaj
Copy link
Member

Mikolaj commented Nov 27, 2021

Agreed, somebody would need to summarize and maintainers would need to decide. However, there's lots of stalled PRs and undecided discussions and that should not stop progress that only comes through merged PRs and user feedback. All the other people involved previously are invited to experiment with the new implementation, tweak and amend afterwards.

Edit: what we can easily do is add all the people from these tickets as reviewers at some point.

@fredefox fredefox force-pushed the stderr branch 10 times, most recently from 3b1d3c5 to 1b28ab3 Compare November 29, 2021 13:55
@fredefox
Copy link
Contributor Author

fredefox commented Dec 3, 2021

I think I managed to actually fix this issue with connecting a bunch of pipes together and waiting in the correct order. Laziness and asynchronicity is a potent coctail. Not for the faint of heart!

Tests can now specify files with a `.stdout` or `.stderr` suffix to
assert where Cabal writes output to. `.out` will continue to work for
testing what output Cabal generates ignoring which file descriptor
that output is sent to.

This commit also simplifies the
`test{,Normalized}{Actual,Expect}File{Out,Stdout,Stderr}` family of
functions
@fredefox
Copy link
Contributor Author

fredefox commented Dec 3, 2021

It should be fairly easy to migrate the tests over. Simply:

  1. Copy all .out files to .stdout
  2. Run all tests
  3. Fix broken tests accordingly.

@fredefox fredefox marked this pull request as ready for review December 3, 2021 22:34
@Mikolaj
Copy link
Member

Mikolaj commented Dec 6, 2021

So is the plan of action to ask for a preliminary review approval and, if obtrained, complete the legwork for the tests? How many relevant tests are passing already? How many are not?

@fredefox
Copy link
Contributor Author

fredefox commented Dec 7, 2021

So is the plan of action to ask for a preliminary review approval and, if obtrained, complete the legwork for the tests?

No I'll make all the tests green before requesting a review. I think that's the only proper way to go. I should've probably marked this as a draft until then. It's just a question of getting around to actually doing it :)

@fredefox fredefox marked this pull request as draft December 7, 2021 10:52
@Mikolaj
Copy link
Member

Mikolaj commented Jan 24, 2022

Hi! How is it going?

@fredefox
Copy link
Contributor Author

fredefox commented Jan 24, 2022

Hi! How is it going?

Hi Mikolaj. I've reached the conclusion that I cannot implement it the way I had envisaged where both stdout, stderr and their collective output are caught and the order of messages are preserved. I don't think it can be done. We should consider how we would want to implement this then. My proposal would be the following:

  1. The tests checks if there are .stdout or .stderr files associated with the test.
  2. If there is, then capture the stdout and stderr output of the program and disregard any .out-files.
  3. Otherwise. Run tests as they are today.

The implication of this is that you cannot test both which output goes where, and the order in which the collective output is generated.

@Mikolaj
Copy link
Member

Mikolaj commented Jan 24, 2022

The implication of this is that you cannot test both which output goes where, and the order in which the collective output is generated.

I don't think we mind the global order often, if at all. Does it mean we could get the same output as today (or at least deterministic enough) in tests, so that we can handle the golden tests method that we use in the biggest test suite(s)?

@fredefox
Copy link
Contributor Author

Does it mean we could get the same output as today (or at least deterministic enough) in tests, so that we can handle the golden tests method that we use in the biggest test suite(s)?

With my sketched design it should be backwards compatible. But you cannot test both modes. So if you want to switch to testing stdout and stderr output you won't be able to test how those are interleaved.

@Mikolaj
Copy link
Member

Mikolaj commented Feb 21, 2022

Fair enough, that's an absolute improvement as is, worth merging IMHO. What about UI? How does the test writer request either normal mode or the seperate stdout and seperate stderr mode?

@fredefox
Copy link
Contributor Author

fredefox commented Mar 2, 2022

What about UI? How does the test writer request either normal mode or the seperate stdout and seperate stderr mode?

So I would imagine that it ought to just switch into the new mode if you there is either a .stdout or a .stderr file present in the test directory. Mind you I still haven't implemented this yet.

@ulysses4ever
Copy link
Collaborator

Hey @fredefox! I'm curious do you think you could finish it any time soon? Great work, and I hope it gets merged!

@ulysses4ever
Copy link
Collaborator

@mergify rebase

@mergify
Copy link
Contributor

mergify bot commented Aug 17, 2022

rebase

❌ Base branch update has failed

Git reported the following error:

Rebasing (1/4)
error: could not apply 7566cf663... Check where Cabal writes output to
Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 7566cf663... Check where Cabal writes output to
Auto-merging cabal-testsuite/src/Test/Cabal/Run.hs
CONFLICT (content): Merge conflict in cabal-testsuite/src/Test/Cabal/Run.hs
Auto-merging cabal-testsuite/src/Test/Cabal/Prelude.hs
Auto-merging cabal-testsuite/src/Test/Cabal/Monad.hs

err-code: E7EF2

@haskell haskell deleted a comment from mergify bot Aug 19, 2022
@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
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.

cabal run should send info logging to stderr not stdout
4 participants