Skip to content

cabal run +RTS ... is confusing #8641

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

Closed
noughtmare opened this issue Dec 25, 2022 · 20 comments
Closed

cabal run +RTS ... is confusing #8641

noughtmare opened this issue Dec 25, 2022 · 20 comments

Comments

@noughtmare
Copy link
Collaborator

noughtmare commented Dec 25, 2022

I've seen multiple reports of issues that were caused by people writing cabal run +RTS ... with the intention of passing those RTS options to the program that they run. Instead cabal itself consumes those RTS options.

Recently, this came up again on reddit.

The obvious solution is that users can just run cabal run -- +RTS .... However, I think this is such a common mistake that it might warrant a custom warning. Would it be possible to show something like the following warning?

$ cabal run +RTS -N
Warning: Your RTS options are applied to cabal itself, not the executable.
         Use 'cabal run -- +RTS -N' to pass the RTS options to your executable.

There are also stackoverflow questions caused by this confusion:

@noughtmare noughtmare changed the title cabal run ... +RTS ... is confusing cabal run +RTS ... is confusing Dec 25, 2022
@BasLaa
Copy link
Collaborator

BasLaa commented Jan 27, 2023

Is this something I could work on? I looked through the codebase, but am having trouble finding the right starting point for this.

@noughtmare
Copy link
Collaborator Author

noughtmare commented Jan 27, 2023

It is a bit tricky because it seems like the +RTS flags are parsed before any Haskell program has started. Or at least the standard System.Environment.getArgs doesn't list the +RTS argument.

So, the first step is to try to write a Haskell program that can tell if it has been given any +RTS ... arguments.

Then the next step would be to integrate that mechanism into cabal.

Edit: I guess getRTSFlags might be useful.

@BasLaa
Copy link
Collaborator

BasLaa commented Jan 27, 2023

Right I see the issue, The +RTS flag is already 'gone' before the Haskell program begins.
Let me try to look into getRTSFlags and see if I can write a sample program which can detect it.

@BasLaa
Copy link
Collaborator

BasLaa commented Jan 27, 2023

From searching around bit it seems pretty doable.
Instead of System.Environment.getArgs, GHC.environment.getFullArgs could be used, which also lists +RTS.

import GHC.Environment (getFullArgs)

main :: IO ()
main = do
    putStrLn "Searching all args..."
    args <- getFullArgs 
    print $ elem "+RTS" args
    return ()

now if I run ./main +RTS, it detects that the flag was passed.

Would this help?

Using getRTSFlags gives the RTS flags (obviously) but doesn't indicate whether +RTS was used to set flags or a different method, like -rtsopts.

@noughtmare
Copy link
Collaborator Author

Yeah, that looks much better than getRTSFlags. Now you still do need to check that the +RTS is not on the right of a -- argument, because then we don't want to show the warning.

As for the actual place where this should go, it should probably only warn if you are using the run command. So, perhaps somewhere near the beginning of Distribution.Client.CmdRun.runAction? Although, I'm not really familiar with the cabal-install codebase either.

@BasLaa
Copy link
Collaborator

BasLaa commented Jan 27, 2023

If I have a detection program in a cabal project, and I run cabal run detect-exe -- +RTS it detects the flag +RTS because the +RTS flag is passed to the program detect-exe. However, if I run cabal run +RTS, it does not detect the flag because the flag is passed to cabal.

For this reason I would think that if I run cabal run +RTS I can detect the flag from within cabal, whereas with cabal run detect-exe -- +RTS I cannot detect it (because it's not passed to cabal). This should be exactly what we want.

Perhaps someone else could give input on where it would be suitable to implement this?

@noughtmare
Copy link
Collaborator Author

noughtmare commented Jan 27, 2023

You can try it and see what happens:

-- T3.hs
import GHC.Environment (getFullArgs)

main :: IO ()
main = do
    putStrLn "Searching all args..."
    args <- getFullArgs 
    print args
$ ./T3 -- +RTS -T
Searching all args...
["./T3","--","+RTS","-T"]
$ ./T3 +RTS -T   
Searching all args...
["./T3","+RTS","-T"]

@BasLaa
Copy link
Collaborator

BasLaa commented Jan 27, 2023

I was talking about the case where you use cabal run. You can try it out: if you run cabal run detect-exe -- +RTS it will be there, if you run cabal run +RTS it won't be there.

The point I was making is that then (likely) within cabal exactly the inverse will be true: if you run cabal run +RTS, then within cabal you can detect the +RTS flag, and If you run cabal run detect-exe -- +RTS it won't be there. This means that we would not need to take care of the case where +RTS is on the right of a --, as then cabal won't detect it.

@noughtmare
Copy link
Collaborator Author

noughtmare commented Jan 27, 2023

But if the cabal program itself runs getFullArgs is then it will surely too get ["cabal","run","detect-exe","--","+RTS"], no?

@BasLaa
Copy link
Collaborator

BasLaa commented Jan 27, 2023

My bad, of course that's correct, I wasn't thinking about it right. That additional check should be doable to implement as well.

@BasLaa
Copy link
Collaborator

BasLaa commented Jan 28, 2023

Perhaps something like this:

import GHC.Environment (getFullArgs)
import Data.List (elemIndex)

main :: IO ()
main = do
    putStrLn "Searching all args..."
    args <- getFullArgs 
    print $ occursBefore args "+RTS" "--"


-- True if x occurs before y, or x occurs and y does not
occursBefore :: (Eq a) => [a] -> a -> a -> Bool
occursBefore xs x y = case (elemIndex x xs, elemIndex y xs) of
                       (Just i, Just j) -> i < j
                       (Just _, _) -> True
                       _ -> False

@BasLaa
Copy link
Collaborator

BasLaa commented Jan 28, 2023

@Mikolaj @ulysses4ever Could you perhaps guide me in where to start implementing this, and if it is worthwhile to implement? It would be an extra check whenever you use cabal run, would that be an issue?

@Mikolaj
Copy link
Member

Mikolaj commented Jan 28, 2023

It's definitely worthwhile. In what sense could that be an issue? E. g., would the very check (not the warnings) be visible to user in any way? Or do you mean vs code complication? If the latter, it's totally worth it.

I think the best way to start is with a PR, a test written using one of the testing harnesses (probably cabal-testsuite is the easiest, but read CONTRIBUTING or test docs to make sure it suits your needs), then start hacking and see if the test passes and if the rest of CI burns. Experiment at will. Expect terrible effects at the start.

Then it should be easier to assist you.

If you are stuck on purely design or motivation matters (trade-offs, warning levels, etc.), perhaps then returning here, to the issue, makes sense. But usually nobody bothers and the PR gets re-purposed.

@fgaz
Copy link
Member

fgaz commented Jan 28, 2023

It doesn't look expensive, I think we can add it directly in CmdRun.hs.

Do we want this check to happen in exec too? I think run is enough, since exec is more for debugging and interacting with tooling

@Mikolaj
Copy link
Member

Mikolaj commented Jan 28, 2023

I know I make similar mistakes with cabal test and cabal benchmark, as silly as it sounds.

@BasLaa
Copy link
Collaborator

BasLaa commented Jan 28, 2023

Thanks, I indeed meant it in terms of code complication. I'll start with a PR and try to write a test.

@Mikolaj
Copy link
Member

Mikolaj commented Feb 2, 2023

Congrats on the PR, @BasLaa! Our convention is that issues are closed when PRs are merged to branch master, so let's not jump the gun.

@Mikolaj Mikolaj reopened this Feb 2, 2023
@BasLaa
Copy link
Collaborator

BasLaa commented Feb 2, 2023

Sorry about that, I meant to close the issue from the other PR I was working on, my bad!

@BasLaa
Copy link
Collaborator

BasLaa commented Feb 25, 2023

Now it's ready to be closed🙂

@ulysses4ever
Copy link
Collaborator

Thank you, @BasLaa!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants