cabal run +RTS warning #8709

BasLaa · 2023-01-29T09:50:34Z

As mentioned in #8641, when running cabal run +RTS, there should be a warning that cabal consumes the +RTS options. This is a first test for that, still requires implementing.

BasLaa · 2023-01-29T09:57:45Z

I added a test in cabal-testsuite/PackageTests/NewBuild/CmdRun but am not sure that this is the correct location for it.

I am also currently having trouble getting the cabal-install testsuites to run alongside the Cabal tests.
Using the command 'cabal-tests --with-cabal=/path/to/cabal', which path exactly should I insert?
Also, I could not figure out how to get just one local test to run (the new one), if someone could give an example of how to do that.

Mikolaj · 2023-01-30T10:26:32Z

The test location and the very test look fine to me at a quick glance.

I assume you read https://github.com/haskell/cabal/blob/master/cabal-testsuite/README.md?

In my bash history I see

/home/mikolaj/r/cabal/dist-newstyle/build/x86_64-linux/ghc-9.0.2/cabal-testsuite-3/build/cabal-tests/cabal-tests --with-cabal /home/mikolaj/.cabal/bin/cabal  cabal-testsuite/PackageTests/NewSdist/DeterministicTrivial/deterministic.test.hs

and in general the path to cabal binary that you give is the binary with your fixes that you just compiled. You can obtain its path with cabal list-bin cabal after compilation.

BasLaa · 2023-01-31T12:35:02Z

I read the README, just couldn't figure out the right paths, turns out something wasn't added to my path properly. I'm now able to run that test locally.

BasLaa · 2023-01-31T14:14:44Z

I added a warning within CmdRun.runAction which seems to pass the new tests. It also broke quite a lot of other tests locally though. Want to see how it does here

BasLaa · 2023-01-31T16:12:12Z

It seems like all the tests pass (except whitespace) with the new warning. Perhaps the helper function I created should go into the Utils file though?

Mikolaj

Great job! Could you add a tiny changelog file, as well (see https://github.com/haskell/cabal/blob/master/CONTRIBUTING.md#changelog)?

Yes, I guess the helper fits one of the Utils files.

noughtmare

@Mikolaj you also suggested the same warning could be added to cabal test and cabal bench. I think it would not be very hard to abstract this warning into a function and use it in multiple places. Is that still something you would like to see?

cabal-install/src/Distribution/Client/CmdRun.hs

BasLaa · 2023-02-02T19:19:41Z

Yeah I agree with what @noughtmare says, if it is desirable for the same warning to appear with other commands, it should be no big issue to abstract this so that it can be reused.

Mikolaj

Thank you for adding the changelog file. So what are your plans? Generalize (slightly?) already in this PR or somehow mark this as future work (e.g., open a ticket about it)?

changelog was graciously added

BasLaa · 2023-02-04T21:07:06Z

If there's a consensus on which commands should have this error I could generalise it here? Otherwise I can push this PR as it is and create a different issue with the suggestion that other commands might want a similar error. Maybe the second approach is clearer?

Mikolaj · 2023-02-06T08:39:56Z

I think doing it right here for cabal test and cabal bench would let you test your abstraction in action. I don't think we need to be exhaustive --- the warning is supposed to educate the user about --, not catch every and all transgression. The three commands seem the most common, so it should do the job. (BTW, I've restarted failed jobs in CI to verify it's just flaky infrastructure.)

noughtmare · 2023-02-06T08:57:33Z

Note that for cabal bench you have to use --benchmark-options="+RTS -N" instead of just the -- +RTS -N separator. I don't know what works for cabal test.

Mikolaj · 2023-02-06T09:02:37Z

Well spotted. For tests it's --test-options. Bah, so we can't monkey-train the user so easily. :) But at least we can train the user to look for a proper option for the command in hand.

BasLaa · 2023-02-06T15:46:53Z

We would still like to give a warning for bench and test when a user runs cabal bench +RTS ... or similar right? So the only thing different would be the warning message?
Then I could pass the command run by the user as an argument and return the correct warning message.

Mikolaj · 2023-02-06T16:04:28Z

Sounds good.

BasLaa · 2023-02-07T11:50:27Z

Now the warning will be given when something like cabal bench +RTS ... or cabal bench -- +RTS is run, for both bench and test. I gave each command its own warning message.

Mikolaj · 2023-02-07T12:04:05Z

cabal-install/src/Distribution/Client/CmdBench.hs

@@ -96,6 +101,10 @@ benchAction flags@NixStyleFlags {..} targetStrings globalFlags = do
               ++ "You may wish to use 'build --only-dependencies' and then "
               ++ "use 'bench'."

+            fullArgs <- getFullArgs
+            when (occursOnlyOrBefore fullArgs "+RTS" "--") $


I'm probably misunderstanding, but does it search for a standalone "--" in a cabal bench commandline? IIRC, "--" is completely ignored by bench and test, so that wouldn't make sense, would it?

Yeah that was why I was wondering in exactly which case we want to give this warning for bench and test. The occursOnlyOrBefore function worked nicely for run as you have either +RTS ... -- (wrong) or -- ... +RTS (right). With bench and test, do we want to give a warning simply when +RTS occurs as a standalone argument, like cabal test +RTS? Then this works, but I could also just make it elem.

It is more difficult to check whether something like --test-options="+RTS" is passed, because it is treated as one argument, so you can't match the string easily. So perhaps for bench and test the best thing is just to check if there is a standalone +RTS. There is the edge case that a user passes something like cabal test --test-options="+RTS" +RTS ... and actually wants to do this, then it would still give the warning even though the user is passing RTS options to the test-suite as well.

I think using elem would be the best. That the warning then also happens when users use cabal test --test-options="+RTS" +RTS ... is not that bad in my opinion.

I think warning for a standalone RTS is fine. I'd just change the warning text for test/build not to say "RTS is standalone instead of done properly", but "RTS found is standalone, which may be intentional to affect cabal, but please note RTS inside proper options is still required for RTS, if your goal is to affect the tested binary". The user may say "doh, silly computer, that's precisely why I keep the other RTS inside proper options", but I hope that's not too confusing.

"Some RTS options were found standalone, which affect cabal instead of the binary. Please note RTS should be inside proper options if your goal is to affect the tested binary. For example, use 'cabal test --test-options="+RTS -N" ' to pass the '-N' RTS option to your binary."
How about this? Then the warning is still relatively succinct.

Works for me. How about "Please note RTS inside proper options suffices..." to also unconfuse a user that thinks both are needed? I'm not sure how realistic such a confusion is, but one should never underestimate users. This has the extra advantage that the user doesn't get an impression that the RTS he's put inside proper options was somehow overlooked by cabal and perhaps is invisible to cabal. In other words, we pretend we see the RTS and just make sure the other RTS is not put there due to the confusion that both are needed.

BasLaa · 2023-02-07T16:32:32Z

I think this is ready now, only the changelog needs an update

Mikolaj

Almost there IMHO.

Mikolaj · 2023-02-07T17:01:43Z

cabal-install/src/Distribution/Client/Utils.hs

+               ++ "to pass the '-N' RTS option to your executable."
+giveRTSWarning "test" = "Some RTS options were found standalone, "
+               ++ "which affect cabal and not the binary. "
+               ++ "Please note that RTS inside proper options suffices "


I'd be more specific: "+RTS inside the --test-options argument". And similarly for bench.

Mikolaj · 2023-02-07T17:04:30Z

cabal-install/src/Distribution/Client/Utils.hs

+               ++ "which affect cabal and not the binary. "
+               ++ "Please note that RTS inside proper options suffices "
+               ++ "if your goal is to affect the tested binary. "
+               ++ "For example, use 'cabal test --test-options=\"+RTS -N\"' "


For silly reasons to do with shells it's in general better to do \"cabal test --test-options=\'+RTS -N'\", because the user can then use double quotes inside the options (and the other way around it doesn't work).

BasLaa · 2023-02-07T22:16:49Z

I'm not sure why the tests are instantly failing? It seems to be something with installation, but I hardly made changes since all tests passed.

Mikolaj · 2023-02-07T23:10:11Z

It seems CI got broken again. I'm just rerunning in case it's transient, but it probably isn't. :|

Mikolaj · 2023-02-07T23:17:06Z

Yes, one test finally passes, but validation tests are borked due to infrastructure problems. It may fix itself, but if not, we'll hack around that. Apologies.

ulysses4ever · 2023-02-10T14:43:03Z

@BasLaa thanks! In general, you don’t have to play catch-up with master — we have a merge bot for that. Currently, we only need a second review for this PR. Hopefully, it will come soon.

BasLaa · 2023-02-10T14:48:00Z

@ulysses4ever good to know!

ulysses4ever

Thanks a lot @BasLaa!

Next step would be to apply a merge label. Either you or Mikolaj (if you don't have the access level necessary) will do it.

mergify · 2023-02-13T06:30:54Z

⚠️ This pull request got rebased on behalf of a random user of the organization.
This behavior will change on the 1st February 2023, Mergify will pick the author of the pull request instead.

To get the future behavior now, you can configure bot_account options (e.g.: bot_account: { author } or update_bot_account: { author }.

Or you can create a dedicated github account for squash and rebase operations, and use it in different bot_account options.

andreasabel · 2023-02-13T16:43:43Z

Thanks for the contribution!

Some merge strategy considerations:

@ulysses4ever wrote:

Thanks a lot @BasLaa!
Next step would be to apply a merge label. Either you or @Mikolaj (if you don't have the access level necessary) will do it.

Looks like the wrong label ("merge" instead of "squash+merge") was chosen, and we ended up with a dozen of micro-commits on master.

External contributors might not know the best choice here, so encouraging them to set the merge label themselves can backfire.

A technical solution would be to limit the "merge" label to administrators and only make the "squash+merge" label available to external contributors. I don't know if this is possible, though.

Or we change our default to: reviewer/administrator sets the merge label after ok from contributor.

BasLaa · 2023-02-13T16:52:17Z

Apologies, now I know for next time!

Mikolaj · 2023-02-13T16:58:04Z

No big problem.

@andreasabel: Indeed, I learned to recommend "squash+merge_me" by default, unless I have the time to look at the number and kind of commits. I'd rather keep the current simple and inclusive setup, as much as the inability to git-bisect hurts us (I wonder if git can easily be persuaded to only consider merge commits). The extra ceremony of giving label permissions lets us honour contributors IMHO and them setting the label underscores their agency, which indeed is crucial given our lack of admins and maintainers.

ulysses4ever · 2023-02-13T16:58:45Z

Indeed, I should have been clearer, mea culpa.

I doubt you can limit which labels a contributor can apply.

andreasabel · 2023-02-13T19:18:57Z

Yes, it is not a big problem. It could be a minor nuisance when cherry-picking or backporting or bisecting, otherwise it is more an aesthetic issue.

The extra ceremony of giving label permissions lets us honour contributors IMHO and them setting the label underscores their agency,

Ok, haven't experienced it like that. I feel "knighted" when I get the 2 approving reviews. ;-) The actual details of the merging ceremony don't move me much; the important part is getting merged.

Mikolaj · 2023-02-13T19:37:15Z

Oh, interesting. I wonder how the other PR authors feel.

It's not only about feelings, though. We really do need contributors to exercise their agency and also act very responsibly, because we have a lack of admin/maintainer workpower. And we need to keep giving authors repo permissions to set labels, etc., because there's not enough active members of cabal community with said permissions to garden the repo in the long run. We celebrate a dire need and a noble duty, not only personal excellence and community camaraderie.

ulysses4ever · 2023-02-13T19:41:30Z

I share and support the ceremony sentiment that Mikolaj mentioned. I understand not everyone does.

BasLaa · 2023-02-13T22:47:45Z

Perhaps from my perspective as a recent contributor; I could have properly read through the labels and realised I should use the squash+merge label and I think it's a mistake you make just once.

It does feel good to be able to label it myself, makes you feel like you're 'in control' of your own PR.

Mikolaj added type: enhancement re: warnings Concerning warnings printed by cabal re: user experience User experience (UX) issue labels Jan 30, 2023

BasLaa marked this pull request as ready for review January 31, 2023 19:24

BasLaa added the attention: needs-review label Feb 1, 2023

Mikolaj previously requested changes Feb 2, 2023

View reviewed changes

Mikolaj requested review from noughtmare and andreasabel February 2, 2023 11:32

noughtmare reviewed Feb 2, 2023

View reviewed changes

cabal-install/src/Distribution/Client/CmdRun.hs Outdated Show resolved Hide resolved

Mikolaj reviewed Feb 4, 2023

View reviewed changes

Mikolaj reviewed Feb 7, 2023

View reviewed changes

Mikolaj approved these changes Feb 7, 2023

View reviewed changes

ulysses4ever approved these changes Feb 11, 2023

View reviewed changes

BasLaa added the merge me Tell Mergify Bot to merge label Feb 11, 2023

mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Feb 13, 2023

BasLaa added 13 commits February 13, 2023 06:30

Add test: RTS warning

0bb3162

add newlines

cc290f0

Add executable in RTS warning test

d3b69bd

Add warning +RTS flag

2ae2694

Fix whitespace

170c862

Add changelog and move utility function

e34ab48

Edit RTS warning message

bef04d7

Edit test output

81756b5

Add warning and tests: bench and run

664fd4e

Use elem and edit tests

f996e60

Remove redundant imports

07d9abb

Update warning and changelog

8719cd6

Fix maximum line length

babf2e2

cocreature force-pushed the cabal-run-RTS-warning branch from 17c233d to babf2e2 Compare February 13, 2023 06:30

mergify bot merged commit 7d4b33f into haskell:master Feb 13, 2023

BasLaa deleted the cabal-run-RTS-warning branch February 16, 2023 13:17

cabal run +RTS warning #8709

cabal run +RTS warning #8709

Conversation

BasLaa commented Jan 29, 2023

BasLaa commented Jan 29, 2023

Mikolaj commented Jan 30, 2023 • edited Loading

BasLaa commented Jan 31, 2023

BasLaa commented Jan 31, 2023

BasLaa commented Jan 31, 2023

Mikolaj left a comment • edited Loading

Choose a reason for hiding this comment

noughtmare left a comment

Choose a reason for hiding this comment

BasLaa commented Feb 2, 2023

Mikolaj left a comment

Choose a reason for hiding this comment

BasLaa commented Feb 4, 2023

Mikolaj commented Feb 6, 2023

noughtmare commented Feb 6, 2023 • edited Loading

Mikolaj commented Feb 6, 2023

BasLaa commented Feb 6, 2023 • edited Loading

Mikolaj commented Feb 6, 2023

BasLaa commented Feb 7, 2023 • edited Loading

Mikolaj Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

BasLaa Feb 7, 2023

Choose a reason for hiding this comment

noughtmare Feb 7, 2023

Choose a reason for hiding this comment

Mikolaj Feb 7, 2023

Choose a reason for hiding this comment

BasLaa Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

Mikolaj Feb 7, 2023

Choose a reason for hiding this comment

BasLaa commented Feb 7, 2023

Mikolaj left a comment

Choose a reason for hiding this comment

Mikolaj Feb 7, 2023

Choose a reason for hiding this comment

Mikolaj Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

BasLaa commented Feb 7, 2023

Mikolaj commented Feb 7, 2023

Mikolaj commented Feb 7, 2023

ulysses4ever commented Feb 10, 2023

BasLaa commented Feb 10, 2023

ulysses4ever left a comment

Choose a reason for hiding this comment

mergify bot commented Feb 13, 2023

andreasabel commented Feb 13, 2023

BasLaa commented Feb 13, 2023

Mikolaj commented Feb 13, 2023

ulysses4ever commented Feb 13, 2023

andreasabel commented Feb 13, 2023

Mikolaj commented Feb 13, 2023

ulysses4ever commented Feb 13, 2023

BasLaa commented Feb 13, 2023

Mikolaj commented Jan 30, 2023 •

edited

Loading

Mikolaj left a comment •

edited

Loading

noughtmare commented Feb 6, 2023 •

edited

Loading

BasLaa commented Feb 6, 2023 •

edited

Loading

BasLaa commented Feb 7, 2023 •

edited

Loading

Mikolaj Feb 7, 2023 •

edited

Loading

BasLaa Feb 7, 2023 •

edited

Loading

Mikolaj Feb 7, 2023 •

edited

Loading