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

Re #7670: WIP: test case for #7248 #7702

Merged
merged 6 commits into from
Oct 7, 2021
Merged

Conversation

andreasabel
Copy link
Member

@andreasabel andreasabel commented Oct 6, 2021

Re #7670: WIP: test case for #7248

Two problems remaining:

  • Warning output is not recorded in the golden value.
  • The correct warning is not produced yet.

Current warning:

Warning: No remote package servers have been specified. Usually you would have one specified in the config file.

Please include the following checklist in your PR:

Please also shortly describe how you tested your change. Bonus points for added tests!

@andreasabel andreasabel requested review from Mikolaj and fgaz October 6, 2021 11:41
@andreasabel andreasabel added the re: error-message Concerning error messages delivered to the user label Oct 6, 2021
@Mikolaj
Copy link
Member

Mikolaj commented Oct 6, 2021

I'm only guessing from a quick code-dive, but how about cabalTest . recordMode RecordAll $? Also, perhaps parameters ["--config-file", conf] and repository specified in the conf file would suffice to warn about the right thing?

@andreasabel
Copy link
Member Author

@Mikolaj wrote:

how about cabalTest . recordMode RecordAll $?

This does record the warnings, but also the call stack, which is unwanted because it is too brittle as a golden value:

Warning: No remote package servers have been specified. Usually you would have one specified in the config file.
CallStack (from HasCallStack):
  withMetadata, called at src/Distribution/Simple/Utils.hs:379:14 in Cabal-3.7.0.0-inplace:Distribution.Simple.Utils
-----BEGIN CABAL OUTPUT-----
Error: cabal: There is no package named 'BNFC'. 
You may need to run 'cabal update' to get the latest list of available packages.
-----END CABAL OUTPUT-----

The call-stack includes line numbers and package versions which are volatile.

The default seems to be recordMode RecordMarked; and the third alternative DoNotRecord, does not sound promising.

I notice that the warning I was triggering is hidden explicitly (since cb5a778):

warn (verboseUnmarkOutput verbosity) $
"No remote package servers have been specified. Usually " ++
"you would have one specified in the config file."

So I was chasing a wild goose.

perhaps parameters ["--config-file", conf] and repository specified in the conf file

For some reason, unpack does not support the --config-file option. Is this a bug?

$ cabal unpack --config-file=FOO BAR
Error: cabal: unrecognized 'unpack' option `--config-file=FOO'

cabal --help lists --config-file as a global option:

Global flags:
 -h, --help                     Show this help text
 -V, --version                  Print version information
 --numeric-version              Print just the version number
 --config-file=FILE             Set an alternate location for the config file
...

But also, according to cabal --help, unpack isn't even a valid command:

$ cabal --help | grep unpack
(NOTHING)

@Mikolaj
Copy link
Member

Mikolaj commented Oct 6, 2021

Edit: see Edit2 below. That was a misunderstanding (and a rather unfriendly parser).

For some reason, unpack does not support the --config-file option. Is this a bug?

Yes, I think so.

I wonder how other tests cope with no config files (and so no repository: foo.org defined). @gbaz, @fgaz, any idea? Should we just echo "repository: foo.org" > ~/.cabal/config and hope it doesn't break subsequent tests?

Edit1: I don't want to use the env var telling where config is, to limit how much we depend on it, even in tests.

Edit2: see #7704, the problem was that the global option should be before command,

Two problems remaining:
- Warning output is not recorded in the golden value.
- The correct warning is not produced yet.

Current warning:

    Warning: No remote package servers have been specified. Usually you would have one specified in the config file.
@andreasabel
Copy link
Member Author

andreasabel commented Oct 6, 2021

@Mikolaj: It seems like I got the test working now, thanks to your feedback.

The warning contains also an unrecognized stanza which is not so desirable, but was good enough to trigger the warning we care about.

See b2cd2d4 for the output before the fix of #7248.

Commits should be merged squashed.

@andreasabel andreasabel marked this pull request as ready for review October 6, 2021 19:42
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.

Looks great. Thank you.

@Mikolaj Mikolaj added the squash+merge me Tell Mergify Bot to squash-merge label Oct 6, 2021
@fgaz
Copy link
Member

fgaz commented Oct 7, 2021

cabal unpack --config-file=FOO BAR

The correct syntax is cabal --config-file=FOO unpack BAR. Global options go before the command

edit: ah I see this was addressed in #7704, great!

@fgaz
Copy link
Member

fgaz commented Oct 7, 2021

unpack isn't even a valid command

unpack is a legacy alias for get

Copy link
Member

@fgaz fgaz left a comment

Choose a reason for hiding this comment

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

the test should use get instead of unpack

@andreasabel
Copy link
Member Author

@fgaz:

the test should use get instead of unpack

Fixed that.

Copy link
Member

@fgaz fgaz left a comment

Choose a reason for hiding this comment

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

Thanks!

I took the liberty of changing x.y.x to repo.invalid

@Mikolaj
Copy link
Member

Mikolaj commented Oct 7, 2021

CI fails due to #7707, so let's merge anyway.

@andreasabel
Copy link
Member Author

How can I merge this now?
(The label "squash+merge me" does not seem magic in the sense that it kicks off a merge process, so what is the merge procedure?)

@Mikolaj
Copy link
Member

Mikolaj commented Oct 7, 2021

@Mergifyio rebase

@Mikolaj
Copy link
Member

Mikolaj commented Oct 7, 2021

I think I need to override the broken CI using admin power.

@mergify
Copy link
Contributor

mergify bot commented Oct 7, 2021

Command rebase: success

Branch already up to date

@Mikolaj Mikolaj merged commit 6bb79dc into haskell:master Oct 7, 2021
@Mikolaj
Copy link
Member

Mikolaj commented Oct 7, 2021

@andreasabel: thank you and please have a look at the single commit that resulted from this. I hope that's what you wished (the label didn't work due to CI failure, but I tried to replicate with the merge button).

@andreasabel
Copy link
Member Author

@Mikolaj: Excellent, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
re: error-message Concerning error messages delivered to the user squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants