Skip to content

Remove local options from global program options #7973

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

Merged
merged 1 commit into from
Feb 28, 2022

Conversation

patrickdoc
Copy link
Collaborator

@patrickdoc patrickdoc commented Feb 13, 2022


Please include the following checklist in your PR:

Please also shortly describe how you tested your change. Bonus points for added tests!


Local options for compiler related flags are 1) applied to all packages and 2) cached. Both of which seem incorrect.

This PR removes those options from the pre-configured program DB. Note that options are still applied correctly here:

<> perPkgOptionMapMappend pkgid packageConfigProgramArgs

There are lots of related issues around weird behavior trying to apply ghc-options:

This PR makes this cabal.project stanza work as intended, applying the ghc-options to all/only local packages:

program-options
    ghc-options: -Werror

and removes the need for this workaround:

package local-foo
  ghc-options: -Werror

package local-bar
  ghc-options: -Werror

@Mikolaj
Copy link
Member

Mikolaj commented Feb 14, 2022

This is amazing! However, people have already got used to the wrong behaviour, so let's make sure we change it once and don't ask them to re-adjust repeatedly. Is the desired semantics completely clear or do we need to ask users, discuss again on the tickets you mentioned, etc.?

@jneira
Copy link
Member

jneira commented Feb 14, 2022

It is somewhat weird this did not break any test. I think we should add some sort of regression test to exercise the change and have evidences it fixes those issues

@jneira
Copy link
Member

jneira commented Feb 14, 2022

It is somewhat weird this did not break any test. I think we should add some sort of regression test to exercise the change and have evidences it fixes those issues

well, as noted in the pr description

This PR makes this cabal.project stanza work as intended, applying the ghc-options to all/only local packages

so it is not so weird there are no tests checking ghc-options are applied to non local packages

Probably this will break quite use cases, it is worth to note you can recover the old behaviour with

package *
  ghc-options: -Werror

if you have a local project. Not sure if there would be a easy one for installing from a tarball or hackage though

@patrickdoc
Copy link
Collaborator Author

Tests added to demonstrate old behavior/verify fix. Before the fix, tests failed with:

    program options local:    FAIL (1.93s)
      tests/IntegrationTests2.hs:1579:
      p
      expected: Nothing
       but got: Just ["-fno-full-laziness"]
      Use -p '/program options/&&/program options local/' to rerun this test only.

Here p is a tarball (remote) package which should not have local options applied to it. But, as the test demonstrates, local options are being applied to remote packages.

I need to double check, but I believe command-line options are also passed into the "local" portion of the project config, which means this would also affect that. I think command line only affecting local packages is the intended behavior, but as you mentioned it might break some use cases.

To recreate old behavior, this still works:

package *
    ghc-options: -Werror

Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

great fix, thanks!

@patrickdoc patrickdoc marked this pull request as ready for review February 26, 2022 23:55
Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

Good job. Let me try to merge this.

@Mikolaj
Copy link
Member

Mikolaj commented Feb 28, 2022

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Feb 28, 2022

rebase

☑️ Nothing to do

  • -closed [:pushpin: rebase requirement]
  • #commits-behind>0 [:pushpin: rebase requirement]

@Mikolaj Mikolaj added merge me Tell Mergify Bot to merge and removed attention: needs-review labels Feb 28, 2022
@mergify mergify bot merged commit c46eb51 into haskell:master Feb 28, 2022
@patrickdoc patrickdoc deleted the local-ghc-options branch April 30, 2022 15:51
ulysses4ever added a commit that referenced this pull request Aug 9, 2022
Simplifies *.project.* files a bit. It wasn't feasible before #7973,
as program-options were applied to dependencies as well, which is undesirable.
ulysses4ever added a commit that referenced this pull request Aug 10, 2022
Simplifies *.project.* files a bit. It wasn't feasible before #7973,
as program-options were applied to dependencies as well, which is undesirable.
ulysses4ever added a commit that referenced this pull request Aug 10, 2022
Simplifies *.project.* files a bit. It wasn't feasible before #7973,
as program-options were applied to dependencies as well, which is undesirable.
ulysses4ever added a commit that referenced this pull request Aug 10, 2022
Simplifies *.project.* files a bit. It wasn't feasible before #7973,
as program-options were applied to dependencies as well, which is undesirable.
ulysses4ever added a commit that referenced this pull request Aug 11, 2022
Simplifies *.project.* files a bit. It wasn't feasible before #7973,
as program-options were applied to dependencies as well, which is undesirable.
ulysses4ever added a commit that referenced this pull request Oct 2, 2022
Simplifies *.project.* files a bit. It wasn't feasible before #7973,
as program-options were applied to dependencies as well, which is undesirable.
andreabedini pushed a commit that referenced this pull request Oct 17, 2022
Simplifies *.project.* files a bit. It wasn't feasible before #7973,
as program-options were applied to dependencies as well, which is undesirable.
ulysses4ever added a commit that referenced this pull request Oct 19, 2022
Simplifies *.project.* files a bit. It wasn't feasible before #7973,
as program-options were applied to dependencies as well, which is undesirable.
alexbiehl pushed a commit to alexbiehl/cabal that referenced this pull request Dec 15, 2022
Simplifies *.project.* files a bit. It wasn't feasible before haskell#7973,
as program-options were applied to dependencies as well, which is undesirable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants