Fix precedence for PATH for build-tools-depends #8972

gbaz · 2023-05-24T20:30:25Z

Resolves #8951 and modifies #8506 in the process. This is very subtle stuff and definitely needs a regression test added as well.

Any changes that could be relevant to users have been recorded in the changelog.
Include manual QA notes if your PR relates to cabal-install.

andreabedini · 2023-05-25T01:38:12Z

Well done troubleshooting this and coming up with a fix!

In the issue, you say (emphasis mine):

the code is very gnarly around how this stuff works, so need to think carefully on best way to fix it.

and right after:

My proposed, janky-as-heck, solution is to have the path from extra-prog-path eagerly added at the end of the useExtraPathEnv option in SetupScriptOptions.

If you allow me, this gives me the impression that your solution somewhat falls short of your own expectations and it makes me wonder if togheter we can find another way.

From your diagnosis:

The deeper diagnosis is that invoke in SetupWrapper.hs is called by v2-build to run the actual build, performed by Cabal-the-library. It sets a PATH including the paths of tools created in build-tool-depends at the head. Then the Setup in Cabal-the-library in turn adds the extra-prog-path stuff even further, in front of that.

Repeating in my own words: cabal-install puts extra-prog-path in the PATH, then passes extra-prog-path to Cabal, which also puts it in the PATH causing confusion.

What if cabal-install did not pass extra-prog-path to Cabal at all? using it only to set the PATH before calling Cabal.
Those who call Cabal directly could use that option on their own (or set PATH as they like). Would this be a bad solution?

This means that the usefulness of 8506 is lost in the non v2-build case

#8506 was to resolve #6304, which was raised as a v2-build problem. Was there even a v1-build issue to solve?

What is the difference between

./Setup.hs configure --extra-prog-path /x/y/z ...

and

PATH=/x/y/z:$PATH ./Setup.hs configure ...

?

I am sure I am missing something here; so I thank you in advance for helping me to understand.

Kleidukos · 2023-06-02T09:43:11Z

@gbaz Would you like my help with fixing the conflicts of your PR?

ffaf1 · 2023-06-17T12:51:52Z

cabal-install/src/Distribution/Client/ProjectPlanning.hs

+      , -- note that the above adds the extra-prog-path directly following the elaborated
+        -- dep paths, so that it overrides the normal path, but _not_ the elaborated extensions
+        -- for build-tools-depends.
+        useExtraEnvOverrides = dataDirsEnvironmentForPlan distdir plan


Comma goes here (for a cleaner diff), I believe.

Yes, please.

ffaf1 · 2023-06-17T12:54:25Z

Cabal/src/Distribution/Simple/Configure.hs

+          ProgramSearchPathDir
+          (fromNubList $ configProgramPathExtra cfg)
+
+-- Note. We try as much as possible to _prepend_ rather than postpend the extra-prog-path


maybe postpend → append

append doesn't specify to front or back

I am not going to debate a native speaker, but “add (something) to the end of a written document.” (def).

Fun! I also don't like how artificial "postpend" sounds but on the other hand maybe extra clarity is justified here...

FWIW I was surprised by "append doesn't specify"; I've always understood it as meaning "to the end of".

ffaf1

good AFAICS

sol

From what I understand, this PR includes unrelated formatting changes, making a review more work than it would need to be.

ffaf1 · 2023-06-17T14:34:02Z

@sol I believe a rebase will fix those.

ulysses4ever · 2023-06-21T14:18:06Z

There's a test failure that I don't understand: https://github.com/haskell/cabal/actions/runs/5320595808/jobs/9634585502?pr=8972

ulysses4ever · 2023-07-01T13:54:25Z

@gbaz do you think you give some more love to this effort? It looks close!

andreasabel · 2024-02-29T17:09:21Z

An end-to-end test that could have verified this PR can be harvested from:

Regression in 3.10 cabal build: the extra-prog-path ends up first in the PATH, shadowing build-tools #9756

gbaz · 2024-02-29T19:13:09Z

this subtle fix was apparently wrongly smashed by #9527 ?

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

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)

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

ulysses4ever added the attention: needs-review label May 27, 2023

ulysses4ever mentioned this pull request Jun 3, 2023

Don't add extra-prog-path to ~/.config/cabal/config (#8951) #8952

Merged

4 tasks

Kleidukos self-assigned this Jun 8, 2023

Kleidukos force-pushed the gb/change-path-order branch 3 times, most recently from b37d3b5 to 4552405 Compare June 8, 2023 19:54

Fix precedence for PATH for build-tools-depends

f33c99b

Kleidukos force-pushed the gb/change-path-order branch from 4552405 to f33c99b Compare June 8, 2023 21:16

Merge branch 'master' into gb/change-path-order

38f31cd

Kleidukos approved these changes Jun 17, 2023

View reviewed changes

ffaf1 reviewed Jun 17, 2023

View reviewed changes

ffaf1 approved these changes Jun 17, 2023

View reviewed changes

sol suggested changes Jun 17, 2023

View reviewed changes

changelog

3fe6c72

Kleidukos added squash+merge me Tell Mergify Bot to squash-merge and removed attention: needs-review labels Jun 18, 2023

mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jun 20, 2023

Merge branch 'master' into gb/change-path-order

4f2c1c5

ulysses4ever added the attention: needs-rebase label Jun 21, 2023

Merge branch 'master' into gb/change-path-order

a3a5f02

mergify bot merged commit 403246c into master Jul 1, 2023

mergify bot deleted the gb/change-path-order branch July 1, 2023 18:23

ulysses4ever removed the attention: needs-rebase label Jul 1, 2023

andreasabel added the re: build-tool Concerning `build-tools` and `build-tool-depends` label Feb 29, 2024

andreasabel mentioned this pull request Feb 29, 2024

Regression in 3.10 cabal build: the extra-prog-path ends up first in the PATH, shadowing build-tools #9756

Closed

andreasabel added the re: extra-prog-path Concerning the `extra-prog-path` configuration option label Feb 29, 2024

alt-romes mentioned this pull request Mar 1, 2024

Find build-tool installed programs before programs in path #9762

Merged

gbaz added a commit to alt-romes/cabal that referenced this pull request Mar 6, 2024

backport part of haskell#8972

db2ac3b

gbaz mentioned this pull request Mar 6, 2024

backport: Find build-tool installed programs before programs in path #9767

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix precedence for PATH for build-tools-depends #8972

Fix precedence for PATH for build-tools-depends #8972

gbaz commented May 24, 2023 •

edited

Loading

andreabedini commented May 25, 2023

Kleidukos commented Jun 2, 2023

ffaf1 Jun 17, 2023

sol Jun 17, 2023

ffaf1 Jun 17, 2023

gbaz Jun 17, 2023

ffaf1 Jun 17, 2023

ulysses4ever Jun 17, 2023

geekosaur Jun 17, 2023

ffaf1 left a comment

sol left a comment

ffaf1 commented Jun 17, 2023

ulysses4ever commented Jun 21, 2023

ulysses4ever commented Jul 1, 2023

andreasabel commented Feb 29, 2024 •

edited

Loading

gbaz commented Feb 29, 2024

Fix precedence for PATH for build-tools-depends #8972

Fix precedence for PATH for build-tools-depends #8972

Conversation

gbaz commented May 24, 2023 • edited Loading

andreabedini commented May 25, 2023

Kleidukos commented Jun 2, 2023

ffaf1 Jun 17, 2023

Choose a reason for hiding this comment

sol Jun 17, 2023

Choose a reason for hiding this comment

ffaf1 Jun 17, 2023

Choose a reason for hiding this comment

gbaz Jun 17, 2023

Choose a reason for hiding this comment

ffaf1 Jun 17, 2023

Choose a reason for hiding this comment

ulysses4ever Jun 17, 2023

Choose a reason for hiding this comment

geekosaur Jun 17, 2023

Choose a reason for hiding this comment

ffaf1 left a comment

Choose a reason for hiding this comment

sol left a comment

Choose a reason for hiding this comment

ffaf1 commented Jun 17, 2023

ulysses4ever commented Jun 21, 2023

ulysses4ever commented Jul 1, 2023

andreasabel commented Feb 29, 2024 • edited Loading

gbaz commented Feb 29, 2024

gbaz commented May 24, 2023 •

edited

Loading

andreasabel commented Feb 29, 2024 •

edited

Loading