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

Change cabal-plan list-bin to cabal list-bin in Makefile #7648

Merged
merged 1 commit into from
Sep 14, 2021

Conversation

fgaz
Copy link
Member

@fgaz fgaz commented Sep 14, 2021

cabal list-bin was introduced in cabal-install 3.4, so it's safe to
assume every cabal developer has it by now. By using that, we
remove the dependency on cabal-plan from the Makefile (though it's still
present in other places).


Please include the following checklist in your PR:

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


I'm not sure if CI will be fine with this though. some jobs are run with older images. on the other hand, CI doesn't use many make commands. let's see...

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.

LGTM

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.

It worked!

@Mikolaj Mikolaj added the merge me Tell Mergify Bot to merge label Sep 14, 2021
cabal list-bin was introduced in cabal-install 3.4, so it's safe to
assume every cabal developer has it by now. By using that, we
remove the dependency on cabal-plan from the Makefile (though it's still
present in other places).
@fgaz fgaz force-pushed the remove-cabal-plan-from-makefile branch from 5c02dfa to 42c0599 Compare September 14, 2021 14:37
@mergify mergify bot merged commit 99e1486 into haskell:master Sep 14, 2021
@fgaz fgaz deleted the remove-cabal-plan-from-makefile branch September 15, 2021 14:37
@andreasabel
Copy link
Member

andreasabel commented Sep 21, 2021

@fgaz
Broke for me (macOS):

$ make cabal-install-test-accept 
rm -rf .ghc.environment.*
cd cabal-testsuite && `cabal list-bin cabal-tests` --with-cabal=`cabal list-bin cabal` --hide-successes -j3 --accept 
Error: cabal: No or multiple targets given

/bin/sh: --with-cabal=/Users/abel/bin/src/cabal/dist-newstyle/build/x86_64-osx/ghc-8.10.7/cabal-install-3.7.0.0/x/cabal/build/cabal/cabal: No such file or directory
make: *** [Makefile:137: cabal-install-test-accept] Error 127

$ cabal list-bin cabal-tests
Error: cabal: The list-bin command is for finding a single binary at once. The
target 'Cabal-tests' refers to the package Cabal-tests-3 which includes the
test suite 'unit-tests', the test suite 'rpmvercmp', the test suite
'parser-tests', the test suite 'no-thunks-test', the test suite
'hackage-tests', the test suite 'custom-setup-tests' and the test suite
'check-tests'.

$ cabal-plan list-bin cabal-tests
/Users/abel/bin/src/cabal/dist-newstyle/build/x86_64-osx/ghc-8.10.7/cabal-testsuite-3/build/cabal-tests/cabal-tests

Could be a problem with case-insensitivity.

@fgaz After our conversation where you mentioned that cabal-plan list-bin was obsolete, I noticed already on my machine that it worked more reliable than cabal list-bin (which in this instance wanted to build all kinds of stuff rather than simply spitting out the name of the binary).

Suggestion: revert this PR (at #7675). (I have to do it anyway on my machine.)

@phadej
Copy link
Collaborator

phadej commented Sep 21, 2021

case insensitive file system strikes again. So dumb.

@fgaz
Copy link
Member Author

fgaz commented Sep 22, 2021

Even then, there's also a problem in... sensible file systems, and when a qualified target is used:

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

This definitely looks like a bug in list-bin, and a bad error message: is it zero, or multiple?

This is not fixed by #7408 by the way.

So yes, let's revert for now. I also opened #7679 to track the bug.

P.S.: yes, I originally only checked a couple of invocations and assumed it worked the same for all others, sorry..

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 type: refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants