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

Add support for rts-options #434

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add support for rts-options #434

wants to merge 1 commit into from

Conversation

sol
Copy link
Owner

@sol sol commented Feb 18, 2021

This PR addresses #237 by adding explicit support for rts-options.

A note on semantics: When -with-rtsopts are specified multiple times then ghc only takes the last occurrence into account. For that reason, when a user wants to add additional rts options within a conditional s/he currently has to reiterate any outer rts options within the conditional, e.g.:

executable:
  ghc-options: '"--with-rtsopts -s"'
  when:
    condition: flag(with-threading)
    ghc-options: '"--with-rtsopts -s -N"'

I implemented rts-options so that they are combined in the same way as most other things we support. The following gives the same result as above:

executable:
  rts-options: -s
  when:
    condition: flag(with-threading)
    rts-options: -N

The tests show how this is mapped to .cabal: https://github.com/sol/hpack/pull/434/files#diff-c65aea3074dd2715b0aba52f9aaa1c5fa81b2ca89099c80150b41c33128a5262R1439-R1464

@snoyberg @quasicomputational @tfausak any thoughts?

@tfausak
Copy link
Collaborator

tfausak commented Feb 18, 2021

Nice!

The tests only show rts-options taking a string. Does it also allow for lists like ghc-options does (IIRC)?

@sol sol requested a review from tfausak February 18, 2021 14:44
@sol sol self-assigned this Feb 18, 2021
@istathar
Copy link

I hope you will consider merging this. It would be a nice addition to hpack.

Copy link
Collaborator

@tfausak tfausak left a comment

Choose a reason for hiding this comment

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

Oh dear, I hope this hasn't been waiting for a review from me this whole time!

rts-options: -s -N
|] `shouldRenderTo` executable_ "foo" [i|
main-is: Main.hs
ghc-options: "-with-rtsopts -s -N"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do these need to be quoted, like -with-rtsopts='-s -N'? Or perhaps the flag can be duplicated, like -with-rtsopts=-s -with-rtsopts=-N.

Copy link
Owner Author

@sol sol Feb 16, 2023

Choose a reason for hiding this comment

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

From reading the motivation for this PR, this it's what I capture: Giving --with-rtsopts multiple times is not accumulative. That's the motivation for this PR in the first place.

Regarding quoting, I assume that what this PR currently does works with Cabal. I haven't tried it again just yet, but pretty sure that I tried when I originally wrote this.

--with-rtsopts='-s -N' may work as well, somebody would need to try. I'm fine with anything that works here, I'm not very opinionated on this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just tried a bunch of things. Here are my findings:

  • -with-rtsopts=-N and -with-rtsopts -N: Works as expected. Doesn't matter if you include the equals sign or not.
  • -with-rtsopts=-N -K1k: Fails because it tries to pass -K1k to GHC rather than to the RTS.
  • -with-rtsopts=-N -with-rtsopts=-K1k: Clobbers the -N flag. Same as setting -with-rtsopts=-K1k.
  • -with-rtsopts='-N' and -with-rtsopts="-N": Fails because it doesn't interpret the quotes, which sends a literal '-N' to the RTS.
  • '-with-rtsopts=-N -K1k' and "-with-rtsopts=-N -K1k": Works! It doesn't matter if you use single or double quotes.

TL;DR: I'd recommend "-with-rtsopts opt1 opt2 ...".

Copy link
Collaborator

Choose a reason for hiding this comment

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

This Cabal issue is very related: haskell/cabal#4818

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks @tfausak for doing all the heavy lifting.

I rarely talk about it in public, but I have had issues with RSI for several years now. when it flares up, I have to stay away from the computer. This is the reason why sometimes I'm less responsive than what I would hope to be.

The TL;DR of it is that I am away from the computer. We still need the read me update, change log entry and version bump. I can try to take care of it eventually. But if somebody wants to lend a hand, then that's still very much appreciated.

Releases are fully automated once the version bump hits main.

Choose a reason for hiding this comment

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

I'd be happy to do the README.md update etc today.

@sol
Copy link
Owner Author

sol commented Feb 16, 2023

@istathar at the moment I don't have free cycles for this. If you want to help move this forward, this are the things we need:

  • double check that it works as expected and optionally do some bikeshedding on the quoting
  • update the read me
  • add an entry to the change log

As a separate PR:

  • bump the minor version

@istathar
Copy link

@sol happy to help with your checklist. I'll see if I can do that this week.

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

Successfully merging this pull request may close these issues.

3 participants