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

Pass RTS arguments from the wrapper to HLS #2228

Closed
wants to merge 4 commits into from

Conversation

cdsmith
Copy link
Contributor

@cdsmith cdsmith commented Sep 21, 2021

Currently, RTS arguments are not passed along from the wrapper to HLS.

I have run into this lately where I'm running HLS on a development VM, and HLS growing its memory usage too far causes the VM to become inaccessible and need to be rebooted. I'd like to pass +RTS -M10G to the HLS process. But this argument is eaten by the wrapper script and not passed along.

Fortunately, there is GHC.Environment.getFullArgs, which includes RTS arguments in the list. I believe that HLS should be using that instead of getArgs.

@cdsmith
Copy link
Contributor Author

cdsmith commented Sep 21, 2021

Oops, this branch includes changes from a previous pull request that was already merged. I'm trying to fix it now.

@cdsmith
Copy link
Contributor Author

cdsmith commented Sep 21, 2021

Okay, this is ready for review now.

@cdsmith
Copy link
Contributor Author

cdsmith commented Sep 21, 2021

On IRC, glguy pointed out that there are RTS options that write to a file, and this will result in both binaries writing to the same file. That's unfortunate, but I cannot think of a better way to do this, given that we can't really prevent the haskell-language-server-wrapper binary from acting according to the RTS options it is passed.

@jneira
Copy link
Member

jneira commented Sep 22, 2021

hmm what about create a dedicate option to bypass the rts options only to hls? or a generic option --server-args to pass it to hls?

@cdsmith
Copy link
Contributor Author

cdsmith commented Sep 22, 2021

I guess I don't think it's worth the complexity. I'm tempted to just say we should document the issue and leave it alone. There are lots of good RTS options to pass along, and only a few bad ones. I think the case that someone is trying to produce runtime statistics to a file is rare enough that we can just warn them not to use the wrapper.

Note that even if we fix this for command line args, the same problem will exist with the GHCRTS environment variable, so then we'd need to do something similar with the environment variable to really fix the problem. It doesn't seem worth going down that rabbit hole.

@jneira
Copy link
Member

jneira commented Sep 23, 2021

Threre is a test fail which might be related with this change:

No 'hie.yaml' found. Try to discover the project type!
    cabal with global ghc:                    FAIL (9.24s)
      test\wrapper\Main.hs:34:
      expected: "9.0.1"
       but got: "[1 of 1] Compiling Main             ( C:\\Users\\runneradmin\\AppData\\Local\\hie-bios\\wrapper-6ce919108ae554ccbea99268a2e8fd8e.hs, C:\\Users\\runneradmin\\AppData\\Local\\Temp\\hie-bios-78290a27edaec680\\Main.o )\nLinking C:\\Users\\runneradmin\\AppData\\Local\\hie-bios\\wrapper-6ce919108ae554ccbea99268a2e8fd8e.exe ...\n9.0.1"
      Use -p '/cabal with global ghc/' to rerun this test only.

will rerun just in case

@cdsmith
Copy link
Contributor Author

cdsmith commented Sep 23, 2021

I don't think it's related, and it looks like it's passing now.

@jneira jneira added the merge me Label to trigger pull request merge label Sep 23, 2021
@duog
Copy link

duog commented Sep 28, 2021

I believe a more robust solution here is to use -rtsopts=ignore or -rtsopts=ignoreall as a ghc-option for the wrapper.

See https://downloads.haskell.org/ghc/latest/docs/html/users_guide/phases.html#ghc-flag--rtsopts[=%E2%9F%A8none|some|all|ignore|ignoreAll%E2%9F%A9]

@jneira jneira removed the merge me Label to trigger pull request merge label Oct 4, 2021
@jneira
Copy link
Member

jneira commented Oct 4, 2021

@cdsmith do you think it worths to follow @duog advide in this pr?

@jneira jneira added the status: needs info Not actionable, because there's missing information label Oct 4, 2021
@cdsmith
Copy link
Contributor Author

cdsmith commented Oct 4, 2021

@cdsmith do you think it worths to follow @duog advide in this pr?

Yes, I think that sounds like a great answer. I'll withdraw this PR

@cdsmith cdsmith closed this Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs info Not actionable, because there's missing information
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants