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

+RTS .. -RTS sections in ghc-options shouldn't affect the artifact hash #5575

Closed
23Skidoo opened this issue Sep 14, 2018 · 7 comments
Closed

Comments

@23Skidoo
Copy link
Member

If I build a project with new-build and then do new-clean && new-build --ghc-options="+RTS -A32M", it will try to rebuild all dependencies as well. This is not necessary and shouldn't happen.

@23Skidoo
Copy link
Member Author

Related: #4247.

/cc @merijn

@merijn
Copy link
Collaborator

merijn commented Sep 14, 2018

Actually, I don't think this is a matter of "RTS options shouldn't affect the hash" as, for executables, they definitely do affect the build output. This actually seems more related to the matter is discussed with @ezyang at some point on whether commandline flags (like --ghc-options) should by default be scoped to everything or just the current package (my argument then and in some ticket I can't find) was that commandline flags should default to only the current project and not all dependencies.

I'm pretty sure there's already a feature request for this somewhere and that'd obsolete the entire difficult task of somehow distinguishing some RTS options as safe and others as not.

@23Skidoo
Copy link
Member Author

Actually, I don't think this is a matter of "RTS options shouldn't affect the hash" as, for executables, they definitely do affect the build output.

Hmm, I don't see how they do. This is about RTS options passed to the GHC executable itself via +RTS (e.g. to make the GHC process use a bigger GC nursery during compilation), not -rtsopts (default RTS options for the resulting exes).

Though if -rtsopts affects only exes and not libraries (I think it affects foreign libraries as well), it'd be nice if it didn't cause libraries to be rebuilt.

@merijn
Copy link
Collaborator

merijn commented Sep 14, 2018

oh, I misinterpreted the problem! This should actually be fairly trivial. It's simply a matter of modifying normaliseArgs (

normaliseGhcArgs :: Maybe Version -> PackageDescription -> [String] -> [String]
) to strip out everything from +RTS to -RTS. But I don't have time to implement that right now.

If anyone else takes it up they should dive into the GHC 8.6 code and check if any flags were added/changed while they're at it, so the supported GHC range can be expanded to include 8.6.1.

@merijn
Copy link
Collaborator

merijn commented Sep 25, 2018

@23Skidoo Should work properly with the PR I just created, but I don't have time to do a lot of testing, so please try dogfooding that one.

@23Skidoo
Copy link
Member Author

Great, thanks, will do.

@23Skidoo
Copy link
Member Author

I tested #5589 with GHC 8.4.1 on a private project, and it seems to work.

23Skidoo added a commit that referenced this issue Nov 8, 2018
Cleans up the `-Werror` check, adds GHC 8.6.1 support and also filters RTS flags, fixing #5575
23Skidoo added a commit that referenced this issue Nov 8, 2018
Cleans up the `-Werror` check, adds GHC 8.6.1 support and also filters RTS flags, fixing #5575

(cherry picked from commit e263e8c)
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

2 participants