Skip to content

fix list-bin to only choose the selected component #7791

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 5 commits into from
Feb 25, 2022

Conversation

larskuhtz
Copy link
Collaborator

@larskuhtz larskuhtz commented Nov 2, 2021

This PR resolves #7679. Currently, at least under certain circumstances, list-bin selects all executable components (including tests and benchmarks) from a package that matches the target selector. This PR fixes this by only selecting the component that match the component name that matches the target selector.

Current behavior:

> cabal list-bin cabal-testsuite:exe:cabal-tests
cabal: No or multiple targets given

Behavior with this PR:

> $(cabal list-bin cabal-install:cabal) list-bin cabal-testsuite:exe:cabal-tests
/Users/lars/Code/github/cabal/dist-newstyle/build/x86_64-osx/ghc-9.0.1/cabal-testsuite-3/build/cabal-tests/cabal-tests

NOTE: during the final selection only the component names but not the component kind are matched. If a package contains two components of different kind, but with the same name (not sure if that is possible), the wrong component might be selected.


Please include the following checklist in your PR:

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

@larskuhtz larskuhtz changed the title fix list-bin to only use the selected component fix list-bin to only choose the selected component Nov 2, 2021
Copy link
Member

@fgaz fgaz left a comment

Choose a reason for hiding this comment

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

Looks good bar comments!

This will need a changelog entry and at least a regression test. Since the logic to test is in listBinAction and not in select*, it will have to be a package test

@Mikolaj
Copy link
Member

Mikolaj commented Nov 2, 2021

The CI failures seem random, so probably everything is fine.

@Mikolaj Mikolaj self-requested a review November 8, 2021 16:02
@Mikolaj
Copy link
Member

Mikolaj commented Nov 8, 2021

@larskuhtz: great, so with a changelog and some minimal test we could merge. Let us know if you need any help with those.

@Mikolaj
Copy link
Member

Mikolaj commented Nov 29, 2021

@larskuhtz: ping?

@mouse07410
Copy link
Collaborator

May I ask what's preventing merge of this PR?

@larskuhtz
Copy link
Collaborator Author

Sorry, for not getting back earlier. I am not familiar with writing tests for cabal commands. I can give it a try, but if someone else wants to do it that would be welcome, too.

@Mikolaj
Copy link
Member

Mikolaj commented Jan 11, 2022

No volunteers, it seems. ;) Please ask questions on Matrix/IRC or here. We'll try to help. I guess a couple of cases (that would fail previously, but work now), with automatically recorded outcomes, in cabal-testsuite/ should be enough and should be relatively easy to copy-paste and amend (and automatically record the results; see the README there). If anybody has a better idea for the test, please yell (I've learned this single test tool and I'm quite ignorant of the other testing mechanisms we do use elsewhere in cabal).

@bacchanalia
Copy link
Collaborator

@larskuhtz if you can create a simple cabal project that replicates the failure I can add it as a test for you

@Mikolaj
Copy link
Member

Mikolaj commented Jan 29, 2022

@bacchanalia: yay, thank you!

@robx
Copy link
Collaborator

robx commented Feb 19, 2022

Just ran into this bug myself, here's a reduced example that should be suitable for a test case:

$ find .
.
./cabal-testsuite
./cabal-testsuite/cabal-testsuite.cabal
./cabal.project
$ cat cabal.project
packages: cabal-testsuite/
$ cat cabal-testsuite/cabal-testsuite.cabal 
cabal-version: 2.2
name:          cabal-testsuite
version:       3

executable cabal-tests

executable setup

custom-setup
$ cabal list-bin cabal-testsuite:exe:cabal-tests
cabal: No or multiple targets given

@jneira
Copy link
Member

jneira commented Feb 19, 2022

Just ran into this bug myself, here's a reduced example that should be suitable for a test case:

ping @bacchanalia

@bacchanalia
Copy link
Collaborator

@larskuhtz I've rebased your pr and added the test case @robx posted. Could you give me permissions to push it to your branch? Or would you prefer I sent you a PR?

@larskuhtz
Copy link
Collaborator Author

@bacchanalia you now have write access to https://github.com/hackage-package-forks/cabal

Thanks a lot for moving this PR forward!

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.

Splendid teamwork.

@Mikolaj
Copy link
Member

Mikolaj commented Feb 21, 2022

Could we have a minimal changelog file, too?

@bacchanalia
Copy link
Collaborator

I added a changelog file. Is this good to merge?

@bacchanalia bacchanalia added merge me Tell Mergify Bot to merge and removed attention: needs-review labels Feb 25, 2022
@Mikolaj
Copy link
Member

Mikolaj commented Feb 25, 2022

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Feb 25, 2022

rebase

❌ Pull request can't be updated with latest base branch changes

Mergify needs the author permission to update the base branch of the pull request.
hackage-package-forks needs to authorize modification on its head branch.
err-code: 9D77C

@Mikolaj
Copy link
Member

Mikolaj commented Feb 25, 2022

@bacchanalia: looks good, but could you rebase once last time?

@bacchanalia
Copy link
Collaborator

rebased

@mergify mergify bot merged commit 1c3cf3a into haskell:master Feb 25, 2022
@mouse07410
Copy link
Collaborator

Guys, great work!

Any idea when it will become a release? I.e, efficient version would include this fix?

@Mikolaj
Copy link
Member

Mikolaj commented Feb 28, 2022

@mouse07410: hi! I bet it will be in cabal 3.8. No release date yet.

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.

cabal list-bin cabal-tests (and others) fails with "No or multiple targets given"
8 participants