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

Apply command line flags to install packages (#8637) #8779

Merged
merged 2 commits into from
Feb 24, 2023

Conversation

patrickdoc
Copy link
Collaborator

@patrickdoc patrickdoc commented Feb 18, 2023


Please include the following checklist in your PR:

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


This PR reverses the effects of #7973 specifically for targets of the "install" command to more closely match expected behavior.

Before this change:

cabal v2-install --ghc-option=-foo hello

would not apply "-foo" to "hello" because "hello" is not a local package. After this change, -foo is applied to "hello" because it is a direct target of the install command, so gets the local package configs added to it.

To test, I ran:

cabal install --ghc-option=-foo alex --verbose

which resulted in:

# Installation of "directory" dependency, no "-foo" flag present
/home/patrick/projects/haskell/cabal/dist-newstyle/build/x86_64-linux/ghc-9.2.3/cabal-install-3.9.0.0/x/cabal/build/cabal/cabal
act-as-setup --build-type=Configure -- configure --verbose=2 --builddir=dist
--ghc
--prefix=/home/patrick/.cabal/store/ghc-9.2.3/directory-1.3.8.0-
<snip>
--ghc-option=-hide-all-packages

# Installation of "alex" package, "-foo" flag present
/home/patrick/projects/haskell/cabal/dist-newstyle/build/x86_64-linux/ghc-9.2.3/cabal-install-3.9.0.0/x/cabal/build/cabal/cabal
act-as-setup --build-type=Simple -- configure --verbose=2 --builddir=dist
--ghc
--prefix=/home/patrick/.cabal/store/ghc-9.2.3/alex...
<snip>
--ghc-option=-hide-all-packages --ghc-option=-foo exe:alex

@Mikolaj
Copy link
Member

Mikolaj commented Feb 21, 2023

@patrickdoc: do you need any help with that one?

@patrickdoc
Copy link
Collaborator Author

@Mikolaj As far as I can tell my change functions as expected, but it is triggering an unexpected pass in the testsuite for this one:
#8744

I read a bit of the conversation around that test, but haven't had a chance to fully understand the original change + the fallout from it. The topics are related enough that I believe I might be fixing the original issue, but it's tough to say.

@Mikolaj
Copy link
Member

Mikolaj commented Feb 22, 2023

@gbaz: any clue?

@gbaz
Copy link
Collaborator

gbaz commented Feb 23, 2023

I think that the test should be revised to pass -- which is the desired behavior. In other words, this pr fixes one of the two issues in #8623 in a more correct way than that change did!

@gbaz
Copy link
Collaborator

gbaz commented Feb 23, 2023

updated the branch with flipping the test expectation

@Mikolaj
Copy link
Member

Mikolaj commented Feb 24, 2023

@patrickdoc: is it fine? anything else? if not, would you set a squash+merge_me or merge_me label? I'd like to backport ASAP

@patrickdoc patrickdoc added the squash+merge me Tell Mergify Bot to squash-merge label Feb 24, 2023
@patrickdoc
Copy link
Collaborator Author

Label added

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.

If Gershom says this is correct then this is correct. @gbaz, do you?

@Mikolaj
Copy link
Member

Mikolaj commented Feb 24, 2023

Let me fast-track the merge process.

@Mikolaj Mikolaj added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Feb 24, 2023
@Mikolaj
Copy link
Member

Mikolaj commented Feb 24, 2023

@mergify rebase

@mergify
Copy link
Contributor

mergify bot commented Feb 24, 2023

⚠️ This pull request got rebased on behalf of a random user of the organization.
This behavior will change on the 1st February 2023, Mergify will pick the author of the pull request instead.

To get the future behavior now, you can configure bot_account options (e.g.: bot_account: { author } or update_bot_account: { author }.

Or you can create a dedicated github account for squash and rebase operations, and use it in different bot_account options.

@mergify
Copy link
Contributor

mergify bot commented Feb 24, 2023

rebase

✅ Branch has been successfully rebased

@Mikolaj
Copy link
Member

Mikolaj commented Feb 24, 2023

@mergify backport 3.10

@mergify
Copy link
Contributor

mergify bot commented Feb 24, 2023

backport 3.10

✅ Backports have been created

@mergify mergify bot merged commit 76670eb into haskell:master Feb 24, 2023
mergify bot pushed a commit that referenced this pull request Feb 24, 2023
* Apply command line flags to install packages (#8637)

* remove expectBroken on NonIgnoredConfigs test

---------

Co-authored-by: gbaz <[email protected]>
(cherry picked from commit 76670eb)
mergify bot added a commit that referenced this pull request Feb 24, 2023
Apply command line flags to install packages (#8637) (backport #8779)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days squash+merge me Tell Mergify Bot to squash-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants