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

Catch exception if git is not installed #10486

Merged
merged 17 commits into from
Nov 6, 2024
Merged

Conversation

noiioiu
Copy link
Contributor

@noiioiu noiioiu commented Oct 28, 2024

This is a trivial pr that should fix a bug in cabal init when git is not installed.


QA Notes

cabal init should work correctly even if git is not installed. This can be tested by running the following commands in an empty directory and answering the prompts:

mkdir /tmp/bin
ln -s "$(which ghc)" "$(which cabal)" /tmp/bin/
PATH=/tmp/bin/ cabal init

@geekosaur
Copy link
Collaborator

Could you fill out one of the templates, please? I don't think this needs a changelog fragment, though.

@ffaf1
Copy link
Collaborator

ffaf1 commented Oct 28, 2024

I wonder if there is a way to test this automatically (if not, just user testing will do).

@geekosaur
Copy link
Collaborator

geekosaur commented Oct 28, 2024

Yes, I think for this one we want (very simple) manual QA notes. I wouldn't trust trying to hide or remove git from a runner environment.

@noiioiu
Copy link
Contributor Author

noiioiu commented Oct 28, 2024

Could you fill out one of the templates, please? I don't think this needs a changelog fragment, though.

Yes, do you have a link to the templates? I can't find them.

@geekosaur
Copy link
Collaborator

https://github.com/haskell/cabal/blob/master/.github/pull_request_template.md

@geekosaur
Copy link
Collaborator

@geekosaur
Copy link
Collaborator

That's not going to work because m isn't guaranteed to have access to Exception (cabal init -n doesn't). I think you will have to arrange to run something like "sh", ["-c", "git … || exit 0"] and deal with the output being empty.

@noiioiu
Copy link
Contributor Author

noiioiu commented Oct 28, 2024

Right, I don't think it's possible to define a MonadCatch instance for PurePrompt.

@noiioiu noiioiu marked this pull request as ready for review October 29, 2024 04:08
@ulysses4ever ulysses4ever linked an issue Oct 31, 2024 that may be closed by this pull request
@ulysses4ever
Copy link
Collaborator

Yes, I think for this one we want (very simple) manual QA notes. I wouldn't trust trying to hide or remove git from a runner environment.

Why not? It should be as simple as the description of this PR shows: "simply" resetting the PATH. I think the testsuite might have a combinator for manipulating PATH.

I don't want to block it for this reason, but having a regression test would be very valuable in this case.

@noiioiu
Copy link
Contributor Author

noiioiu commented Oct 31, 2024

Added tests to the checklist. I'm not sure how or where to add the test though.

Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

Thanks for reporting and taking this on!

An integration test manipulating PATH can be found here: https://github.com/haskell/cabal/blob/63c486a1a65de599fa435c0cbf11ad85725f3c6c/cabal-testsuite/PackageTests/BuildTools/Foreign/setup.test.hs

General setup for integration tests is described here: https://github.com/haskell/cabal/blob/63c486a1a65de599fa435c0cbf11ad85725f3c6c/cabal-testsuite/README.md

Good luck!

@geekosaur
Copy link
Collaborator

My worry with $PATH is that, on Linux at least, the part that needs to be removed is /usr/bin.

@ulysses4ever
Copy link
Collaborator

@geekosaur that's fair. Since it's very local (ideally, just for a single test), it might work.

@noiioiu noiioiu requested a review from ulysses4ever October 31, 2024 11:32
Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

Brilliant!

Have you tried to run the test without your patch? That would be a good sanity check to see that it fails since it's a somewhat non-trivial test.

@noiioiu
Copy link
Contributor Author

noiioiu commented Oct 31, 2024

Brilliant!

Have you tried to run the test without your patch? That would be a good sanity check to see that it fails since it's a somewhat non-trivial test.

I ran it without the patch on a different branch (forked from 3.10) since I couldn't build master on my computer. It failed with the posix_spawnp error.

@geekosaur
Copy link
Collaborator

since I couldn't build master on my computer.

Uh, what happened? Is this something likely to affect other people?

@ulysses4ever
Copy link
Collaborator

I assume the posix_spawn is what we expect to see without the patch (as reported in #10484) --- that's great!

While failing to build master is concerning, it shouldn't block the current patch. Please feel free to open a new issue explaining the trouble with master.

@Kleidukos Kleidukos added the attention: needs-manual-qa PR is destined for manual QA label Oct 31, 2024
@Kleidukos Kleidukos self-assigned this Oct 31, 2024
@Kleidukos
Copy link
Member

Assigning myself for manual QA once this lands on master

@noiioiu
Copy link
Contributor Author

noiioiu commented Oct 31, 2024

since I couldn't build master on my computer.

Uh, what happened? Is this something likely to affect other people?

I think it might be because I was using cabal 3.10.3.0.

I assume the posix_spawn is what we expect to see without the patch (as reported in #10484) --- that's great!

Yes, it's the same error.

@geekosaur
Copy link
Collaborator

Older cabal-install should be able to install newer ones, so that sounds like a bug. Please file a new issue including as many details about what you did and what happened as you can.

@ulysses4ever ulysses4ever added the squash+merge me Tell Mergify Bot to squash-merge label Nov 2, 2024
@mergify mergify bot added ready and waiting Mergify is waiting out the cooldown period merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days labels Nov 2, 2024
@mergify mergify bot merged commit e7bc62b into haskell:master Nov 6, 2024
42 checks passed
@ulysses4ever
Copy link
Collaborator

@mergify backport 3.14

Copy link
Contributor

mergify bot commented Nov 6, 2024

backport 3.14

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Nov 6, 2024
* Catch exception if git is not installed

* fix formatting

* change type from IO to m

* add maybeReadProcessWithExitCode

* use maybeReadProcessWithExitCode

* disambiguate P.catch

* add TypeApplications pragma

* add missing arguments

* Add changelog entry

* Add test for `cabal init` when `git` is not installed

* Remove withSourceCopyDir from test

* Remove withSourceCopyDir from test

* Remove configure and build from test

* Remove assert

* Skip test on windows

---------

Co-authored-by: noiioiu <[email protected]>
(cherry picked from commit e7bc62b)
mergify bot added a commit that referenced this pull request Nov 6, 2024
Backport #10486: Catch exception if git is not installed
@geekosaur
Copy link
Collaborator

@mergify backport 3.12

Copy link
Contributor

mergify bot commented Nov 6, 2024

backport 3.12

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Nov 6, 2024
* Catch exception if git is not installed

* fix formatting

* change type from IO to m

* add maybeReadProcessWithExitCode

* use maybeReadProcessWithExitCode

* disambiguate P.catch

* add TypeApplications pragma

* add missing arguments

* Add changelog entry

* Add test for `cabal init` when `git` is not installed

* Remove withSourceCopyDir from test

* Remove withSourceCopyDir from test

* Remove configure and build from test

* Remove assert

* Skip test on windows

---------

Co-authored-by: noiioiu <[email protected]>
(cherry picked from commit e7bc62b)

# Conflicts:
#	cabal-install/src/Distribution/Client/Init/Types.hs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention: needs-backport 3.14 attention: needs-manual-qa PR is destined for manual QA cabal-install: cmd/init merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days ready and waiting Mergify is waiting out the cooldown period squash+merge me Tell Mergify Bot to squash-merge
Projects
Status: To Test
Development

Successfully merging this pull request may close these issues.

cabal init crashes if git is not installed
5 participants