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

Fix #8400 by marking output of cabal list-bin #8670

Merged
merged 3 commits into from
Jan 16, 2023
Merged

Fix #8400 by marking output of cabal list-bin #8670

merged 3 commits into from
Jan 16, 2023

Conversation

andreasabel
Copy link
Member

Fix #8400 by marking output of cabal list-bin

The regular output of cabal list-bin should go to stdout always, but be marked for the sake of the testsuite.

If this PR is merged quickly, we do not need:

Replaces:

The regular output of `cabal list-bin` should go to stdout always, but
be marked for the sake of the testsuite.
@andreasabel andreasabel added cabal-install: cmd/list-bin regression on master Regression that is unreleased and needs to be fixed before release labels Jan 13, 2023
@andreasabel
Copy link
Member Author

@bacchanalia : I do not understand why the following assertOutputContains succeeds:

res <- cabal' "list-bin" ["script.hs"]
env <- getTestEnv
cacheDir <- getScriptCacheDirectory $ testCurrentDir env </> "script.hs"
assertOutputContains cacheDir res

The output is just this:
Resolving dependencies...
Build profile: -w ghc-<GHCVER> -O1
In order, the following will be built:
- fake-package-0 (exe:cabal-script-script.hs) (first run)

I'd like to change the assertion into something that would fail if list-bin didn't print the path to the script, but first I'd like to understand why the current check does not fail.

Copy link
Collaborator

@athas athas left a comment

Choose a reason for hiding this comment

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

Looks correct to me.

@Mikolaj
Copy link
Member

Mikolaj commented Jan 13, 2023

That's a puzzling CI failure. Looks like a random corruption, but I can't rule out it's somehow caused by this PR. @andreasabel: could you have a look?

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

LGTM, except for the test result on Windows, which may be wrong due to Windows path separators that, for whatever reason, are not translated properly.

@andreasabel
Copy link
Member Author

andreasabel commented Jan 13, 2023

LGTM, except for the test result on Windows, which may be wrong due to Windows path separators that, for whatever reason, are not translated properly.

Thanks, good catch: https://github.com/haskell/cabal/actions/runs/3910085986/jobs/6681874680#step:18:138

dist\build\x86_64-windows\ghc-9.4.2\SelectedComponent-1.0.0\build\testexe\testexe.exe

The path is printed with backslashes under Windows, need to fix my acceptance criterion.

@Mikolaj Mikolaj requested a review from Cmdv January 13, 2023 11:25
Copy link
Collaborator

@Cmdv Cmdv left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

Good to know about withOutputMarker, and thank you for fixing the issue!

@andreasabel andreasabel added the squash+merge me Tell Mergify Bot to squash-merge label Jan 14, 2023
@andreasabel andreasabel self-assigned this Jan 14, 2023
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jan 16, 2023
@mergify mergify bot merged commit 55d6f32 into master Jan 16, 2023
@andreasabel andreasabel deleted the issue-8400 branch January 16, 2023 16:36
@@ -133,7 +133,14 @@ listbinAction flags@NixStyleFlags{..} args globalFlags = do

case binfiles of
[] -> die' verbosity "No target found"
[exe] -> info normal exe
[exe] -> putStr $ withOutputMarker verbosity exe
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not putStrLn?

Copy link
Member

Choose a reason for hiding this comment

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

Ouch, all tests had EOF at the end, so this wasn't visible.

Copy link
Member Author

@andreasabel andreasabel Jan 20, 2023

Choose a reason for hiding this comment

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

Yeah, putStrLn would be better in the normal operation. I guess I got pulled towards putStr because the withOutputMarker puts the EOL character there:

withOutputMarker _ xs =
"-----BEGIN CABAL OUTPUT-----\n" ++
withTrailingNewline xs ++
"-----END CABAL OUTPUT-----\n"

So maybe the correct way is:

  putStr $ withOutputMarker verbosity $ exe ++ "\n"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cabal-install: cmd/list-bin merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days regression on master Regression that is unreleased and needs to be fixed before release squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cabal list-bin is broken cabal list-bin does not produce marked output
6 participants