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

Fix extra-prog-path propagation in the codebase. #9527

Merged
merged 2 commits into from
Jan 18, 2024

Conversation

jasagredo
Copy link
Collaborator

@jasagredo jasagredo commented Dec 15, 2023

Fixes #9519.
Fixes #8391.

This allows finding system executables in:

  • cabal exec will find whatever executable
  • cabal build when cloning source-repository-packages will find git
  • cabal get if invoking git (with -s for example) will work
  • cabal run'ed executable inherits the path

In particular this fixes PATH issues when running MinGW cabal in PowerShell.

Paths added are also now logged in info verbosity.

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • Manual QA notes have been included.
  • Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

QA Notes

In a Windows system with MSYS2/MinGW installed, either system-wide or by ghcup.

Running the following in a PowerShell with previous cabal should fail:

PS C:\Users\Javier\code> cabal exec sh
Error: cabal-3.10.2.0.exe: The program 'sh' is required but it could not be
found.

And now it should work:

PS C:\Users\Javier\code> C:\\Users\\Javier\\code\\cabal\\dist-newstyle\\build\\x86_64-windows\\ghc-9.6.3\\cabal-install-3.11.0.0\\x\\cabal\\build\\cabal\\cabal.exe exec sh
sh-5.2$

Also when you have source-repository-package stanzas in your cabal.project, it doesn't work:

PS C:\Users\Javier\code\foo> cabal build
Error: cabal-3.10.2.0.exe: The program 'git' is required but it could not be
found.

And after this PR it does:

PS C:\Users\Javier\code\foo> C:\\Users\\Javier\\code\\cabal\\dist-newstyle\\build\\x86_64-windows\\ghc-9.6.3\\cabal-install-3.11.0.0\\x\\cabal\\build\\cabal\\cabal.exe build
Warning: this is a debug build of cabal-install with assertions enabled.
Cloning into '/c/Users/Javier/code/foo/dist-newstyle/src/quickchec_-904b1af1c882c12e'...
remote: Enumerating objects: 6266, done.
...

cabal get -s used to fail:

PS C:\Users\Javier\code\tasty> cabal get quickcheck-state-machine -s --verbose=3
...
Searching for git in path.
Cannot find git on the path
CallStack (from HasCallStack):
  withMetadata, called at src\\Distribution\\Simple\\Utils.hs:368:14 in Cabal-3.10.2.1-inplace:Distribution.Simple.Utils
Error: cabal-3.10.2.0.exe: The program 'git' is required but it could not be
found.

not anymore:

PS C:\Users\Javier\code\tasty> C:\\Users\\Javier\\code\\cabal\\dist-newstyle\\build\\x86_64-windows\\ghc-9.6.3\\cabal-install-3.11.0.0\\x\\cabal\\build\\cabal\\cabal.exe get quickcheck-state-machine-0.7.0 -s --verbose=3
...
Searching for git in path.
Found git at C:\msys64\usr\bin\git.exe
Running: "C:\msys64\usr\bin\git.exe" "--version"
C:\msys64\usr\bin\git.exe is version 2.43.0

The PATH printed in the verbose mode used to sometimes have \n interleaved, can be seen with cabal get -s ... --verbose=3:

("PATH","C:\\ghcup\\bin,\nC:\\cabal\\bin,\nC:\\msys64\\mingw64\\bin,\nC:\\msys64\\usr\\bin;C:\\Program Files\\...")

not anymore after this change.

@jasagredo jasagredo requested review from jneira and fendor December 15, 2023 16:14
@jasagredo jasagredo force-pushed the js/fix-extra-paths-propagation branch from 9993cbd to 38a2d4a Compare December 15, 2023 16:24
@jasagredo jasagredo marked this pull request as ready for review December 15, 2023 16:24
@jasagredo jasagredo requested a review from hasufell December 15, 2023 16:25
@jasagredo
Copy link
Collaborator Author

It does seem I went a bit over the board. cabal get seems to work without this modification and so does compilation of network (which invokes other executables), so maybe the fix I did in those was not really needed.

@jasagredo
Copy link
Collaborator Author

Ah no actually cabal get without this change shows:

("PATH","C:\\ghcup\\bin,\nC:\\cabal\\bin,\nC:\\msys64\\mingw64\\bin,\nC:\\msys64\\usr\\bin;C:\\Program Files\\...")

(notice the \ns). After this change it is ok.

@jasagredo
Copy link
Collaborator Author

jasagredo commented Dec 16, 2023

And in fact get doesn't always work!

PS C:\Users\Javier\code\tasty> cabal get quickcheck-state-machine -s --verbose=3
...
Searching for git in path.
Cannot find git on the path
CallStack (from HasCallStack):
  withMetadata, called at src\\Distribution\\Simple\\Utils.hs:368:14 in Cabal-3.10.2.1-inplace:Distribution.Simple.Utils
Error: cabal-3.10.2.0.exe: The program 'git' is required but it could not be
found.

versus the new version

PS C:\Users\Javier\code\tasty> C:\\Users\\Javier\\code\\cabal\\dist-newstyle\\build\\x86_64-windows\\ghc-9.6.3\\cabal-install-3.11.0.0\\x\\cabal\\build\\cabal\\cabal.exe get quickcheck-state-machine-0.7.0 -s --verbose=3
...
Searching for git in path.
Found git at C:\msys64\usr\bin\git.exe
Running: "C:\msys64\usr\bin\git.exe" "--version"
C:\msys64\usr\bin\git.exe is version 2.43.0
...

@jasagredo jasagredo force-pushed the js/fix-extra-paths-propagation branch from 38a2d4a to 4d6dfd6 Compare December 16, 2023 15:57
@jasagredo
Copy link
Collaborator Author

jasagredo commented Dec 16, 2023

Fixed it also for cabal run!

PS C:\Users\Javier\code\auxxx> cat .\app\Main.hs
module Main where

import System.Process

main :: IO ()
main = callCommand "git --version"
PS C:\Users\Javier\code\auxxx> cabal run
'git' is not recognized as an internal or external command,
operable program or batch file.
PS C:\Users\Javier\code\auxxx> C:\\Users\\Javier\\code\\cabal\\dist-newstyle\\build\\x86_64-windows\\ghc-9.6.3\\cabal-install-3.11.0.0\\x\\cabal\\build\\cabal\\cabal.exe run
...
git version 2.43.0

@jasagredo jasagredo force-pushed the js/fix-extra-paths-propagation branch 2 times, most recently from 50a61e8 to 6d48309 Compare December 18, 2023 10:43
fendor
fendor previously requested changes Dec 19, 2023
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

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

Idea looks good to me, some small nitpicking.

I am going to manually check the change later.

cabal-install/src/Distribution/Client/CmdExec.hs Outdated Show resolved Hide resolved
cabal-install/src/Distribution/Client/Get.hs Outdated Show resolved Hide resolved
cabal-install/src/Distribution/Client/VCS.hs Show resolved Hide resolved
cabal-install/src/Distribution/Client/VCS.hs Show resolved Hide resolved
cabal-install/src/Distribution/Client/VCS.hs Outdated Show resolved Hide resolved
@jasagredo jasagredo force-pushed the js/fix-extra-paths-propagation branch from 6d48309 to cbcfb6b Compare December 19, 2023 21:57
@jasagredo jasagredo force-pushed the js/fix-extra-paths-propagation branch 2 times, most recently from 677367f to 5c51f82 Compare December 22, 2023 01:13
@jasagredo jasagredo requested a review from fendor December 22, 2023 01:18
@jasagredo jasagredo force-pushed the js/fix-extra-paths-propagation branch from 5c51f82 to c03caf8 Compare December 22, 2023 01:24
@jasagredo
Copy link
Collaborator Author

I have reworked the solution a bit, in the same spirit as before but more principled. The commit message should give some guidance for the review.

@jasagredo jasagredo force-pushed the js/fix-extra-paths-propagation branch from c03caf8 to 4cf508f Compare December 27, 2023 11:18
@alt-romes
Copy link
Collaborator

Great! Looks good to me.

@jasagredo jasagredo mentioned this pull request Jan 8, 2024
4 tasks
@jasagredo
Copy link
Collaborator Author

Each command action is its own function, so there is no better way right now to do this I think without breaking many things. So I'd rather leave it as is.

@jasagredo jasagredo force-pushed the js/fix-extra-paths-propagation branch from b0a2344 to 981111c Compare January 16, 2024 18:38
@jasagredo jasagredo added the merge me Tell Mergify Bot to merge label Jan 16, 2024
@jasagredo jasagredo force-pushed the js/fix-extra-paths-propagation branch from 981111c to eafdd0e Compare January 16, 2024 18:39
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jan 18, 2024
jasagredo and others added 2 commits January 18, 2024 18:42
Extra prog paths were being handled in many different ways all thorugh
the codebase. This PR introduces a unified way to look at them.

Aiming for traceability, the addition of extra paths is now traced via
`logExtraProgramSearchPath`. All appearances of
`modifyProgramSearchPath` are replaced with `appendProgramSearchPath`
which traces the added paths.

`progInvokePathEnv` was only being set by GHC for some paths to
executables in components and only under certain circumstances. Now
every `ghcInvocation` sets the extra paths directly into `pkgInvokeEnv`.

In particular this fixes PATH issues when running MinGW cabal in
PowerShell, as usually for other OSes the system path contains most of
the expected directories.
Similarly to CmdExec and CmdTest, get paths to all dependency binaries
and add those to PATH. Unlike CmdExec, add just the explicitly required
paths.
@Mikolaj Mikolaj force-pushed the js/fix-extra-paths-propagation branch from eafdd0e to f06195d Compare January 18, 2024 18:42
@mergify mergify bot merged commit 4d35045 into haskell:master Jan 18, 2024
50 checks passed
@jasagredo jasagredo deleted the js/fix-extra-paths-propagation branch January 18, 2024 22:35
@jasagredo
Copy link
Collaborator Author

@mergify backport 3.10

Copy link
Contributor

mergify bot commented Jan 18, 2024

backport 3.10

✅ Backports have been created

mergify bot added a commit that referenced this pull request Jan 22, 2024
Fix extra-prog-path propagation in the codebase. (backport #9527)
@andreasabel andreasabel added the re: extra-prog-path Concerning the `extra-prog-path` configuration option label Feb 29, 2024
alt-romes added a commit to alt-romes/cabal that referenced this pull request Mar 1, 2024
We must consider the path to the installed build-tool before the path to
existing versions of the build tool in paths such as `extra-prog-path`
or in the system path.

This was previously fixed by haskell#8972 but undone by haskell#9527.

This also renames `appendProgramSearchPath` to
`prependProgramSearchPath` to describe correctly what that function
does.

Fixes haskell#9756
alt-romes added a commit to alt-romes/cabal that referenced this pull request Mar 1, 2024
We must consider the path to the installed build-tool before the path to
existing versions of the build tool in paths such as `extra-prog-path`
or in the system path.

This was previously fixed by haskell#8972 but undone by haskell#9527.

This also renames `appendProgramSearchPath` to
`prependProgramSearchPath` to describe correctly what that function
does.

Fixes haskell#9756
alt-romes added a commit to alt-romes/cabal that referenced this pull request Mar 1, 2024
We must consider the path to the installed build-tool before the path to
existing versions of the build tool in paths such as `extra-prog-path`
or in the system path.

This was previously fixed by haskell#8972 but undone by haskell#9527.

This also renames `appendProgramSearchPath` to
`prependProgramSearchPath` to describe correctly what that function
does.

Fixes haskell#9756
alt-romes added a commit to alt-romes/cabal that referenced this pull request Mar 1, 2024
We must consider the path to the installed build-tool before the path to
existing versions of the build tool in paths such as `extra-prog-path`
or in the system path.

This was previously fixed by haskell#8972 but undone by haskell#9527.

This also renames `appendProgramSearchPath` to
`prependProgramSearchPath` to describe correctly what that function
does.

Fixes haskell#9756
alt-romes added a commit to alt-romes/cabal that referenced this pull request Mar 1, 2024
We must consider the path to the installed build-tool before the path to
existing versions of the build tool in paths such as `extra-prog-path`
or in the system path.

This was previously fixed by haskell#8972 but undone by haskell#9527.

This also renames `appendProgramSearchPath` to
`prependProgramSearchPath` to describe correctly what that function
does.

Fixes haskell#9756
Mikolaj pushed a commit to alt-romes/cabal that referenced this pull request Mar 3, 2024
We must consider the path to the installed build-tool before the path to
existing versions of the build tool in paths such as `extra-prog-path`
or in the system path.

This was previously fixed by haskell#8972 but undone by haskell#9527.

This also renames `appendProgramSearchPath` to
`prependProgramSearchPath` to describe correctly what that function
does.

Fixes haskell#9756
mergify bot pushed a commit that referenced this pull request Mar 4, 2024
We must consider the path to the installed build-tool before the path to
existing versions of the build tool in paths such as `extra-prog-path`
or in the system path.

This was previously fixed by #8972 but undone by #9527.

This also renames `appendProgramSearchPath` to
`prependProgramSearchPath` to describe correctly what that function
does.

Fixes #9756

(cherry picked from commit 84c30c2)
erikd pushed a commit to erikd/cabal that referenced this pull request Apr 22, 2024
We must consider the path to the installed build-tool before the path to
existing versions of the build tool in paths such as `extra-prog-path`
or in the system path.

This was previously fixed by haskell#8972 but undone by haskell#9527.

This also renames `appendProgramSearchPath` to
`prependProgramSearchPath` to describe correctly what that function
does.

Fixes haskell#9756
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention: needs-backport 3.10 attention: needs-manual-qa PR is destined for manual QA merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge re: extra-prog-path Concerning the `extra-prog-path` configuration option
Projects
Status: Testing Approved
6 participants