fix: expand `~/bin` in PATH and ensure consistency in subshells, for #6627 #6689

stasadev · 2024-11-05T14:44:37Z

The Issue

These comments:

docs: Show how to use a post-start hook to set the composer version #6688 (comment)
docs: Show how to use a post-start hook to set the composer version #6688 (comment)

And:

Inside the subshells, some strange things happen:

$ ddev ssh
stas@d11-web:/var/www/html$ echo $PATH
~/bin:/var/www/html/vendor/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/var/www/html/bin
stas@d11-web:/var/www/html$ bash
stas@d11-web:/var/www/html$ echo $PATH
~/bin:/var/www/html/vendor/bin:~/bin:/var/www/html/vendor/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/var/www/html/bin:/var/www/html/bin
stas@d11-web:/var/www/html$ bash
stas@d11-web:/var/www/html$ echo $PATH
~/bin:/var/www/html/vendor/bin:~/bin:/var/www/html/vendor/bin:~/bin:/var/www/html/vendor/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/var/www/html/bin:/var/www/html/bin:/var/www/html/bin

For:

fix: composer revert Cmd back to RawCmd and add PATH env, for #6604, fixes #6628 #6627

How This PR Solves The Issue

It turned out that when you pass PATH=~/bin:... as env, it doesn't understand what ~ means. This PR replaces ~ with $HOME.
export PATH=... was added every time you go into the subshell, which led to more and more entries being added to the $PATH. I don't know if it affects anything, but it looks pretty weird, so I fixed it.

Manual Testing Instructions

Using DDEV HEAD, install composer:

ddev composer require composer/composer:v2.8.0 --dev

Add this to .ddev/config.yaml:

hooks:
    post-start:
        - exec: mkdir -p ~/bin && ln -sf /usr/local/bin/composer ~/bin/composer

ddev restart
ddev composer -V # should be the same as in /usr/local/bin/composer

Try echo $PATH in subshells:
```
ddev ssh
echo $PATH
bash
echo $PATH
bash
echo $PATH
```
It should be the same each time.

Automated Testing Overview

Release/Deployment Notes

rfay · 2024-11-05T14:49:56Z

Why was ~/bin/composer in use here anyway (in addition to vendor/bin/composer and /usr/local/bin/composer)?

github-actions · 2024-11-05T14:51:04Z

Download the artifacts for this pull request:

See Testing a PR.

stasadev · 2024-11-05T14:53:36Z

Why was ~/bin/composer in use here anyway (in addition to vendor/bin/composer and /usr/local/bin/composer)?

It's the first item in the $PATH, I guess we should respect it, and let people to do such overrides for composer:

ddev/containers/ddev-webserver/ddev-webserver-base-files/etc/bashrc/commandline-addons.bashrc

Line 1 in 92cd790

export PATH="~/bin:$PATH:/var/www/html/bin"

rfay · 2024-11-05T15:02:28Z

Sorry, my question is not whether it should work, but why we would have used it as a composer instance. As we've seen with the ddev binary, having it in multiple places can only lead to confusion. So I was asking why we would ever put composer in ~/bin to begin with.

stasadev · 2024-11-05T15:08:56Z

why we would ever put composer in ~/bin to begin with.

To use a different composer, as @deviantintegral said in #6688 (comment) what if people have composer/composer in their composer.json, but they use it for a different purpose.

Even though I don't use it this way, I see a problem here as well, for example, after installing it, you can't uninstall it in the usual way because it can't uninstall itself:

ddev composer require composer/composer:v2.8.0 --dev
ddev composer remove composer/composer --dev # throws error

rfay

Minor suggestions.

I generally don't think we should add every workaround for every obscure case we encounter to the docs, because they get cluttered. I usually point to Stack Overflow for cases like that.

rfay · 2024-11-06T18:38:11Z

docs/content/users/configuration/config.md

@@ -95,7 +95,13 @@ Composer version for the web container and the [`ddev composer`](../usage/comman
 | :octicons-file-directory-16: project | `2` | Can be `2`, `1`, or empty (`""`) for latest major version at container build time.<br><br>Can also be a minor version like `2.2` for the latest release of that branch, an explicit version like `1.0.22`, or a keyword like `stable`, `preview` or `snapshot`. See Composer documentation.

 !!!note "If your `composer.json` requires `composer/composer` that version will be used instead"
-    If your project `composer.json` includes `composer/composer`, then the version specified there will normally be used instead of any version specified by `composer_version`, since `vendor/bin/composer` will come first in the in-container `$PATH`.
+    If your project `composer.json` includes `composer/composer`, then the version specified there will normally be used instead of any version specified by `composer_version`, since `vendor/bin/composer` will come first in the in-container `$PATH`. However, if you prefer to use the version specified by `composer_version`, add the following hook to your `.ddev/config.yaml`:


This seems like a very unusual workaround. Why would people not want to use the version they specify in their composer.json?

I worry that adding too much detail in places like this just makes it so people can't make it through the docs, or find the right thing when they need it.

Why would people not want to use the version they specify in their composer.json?

It's less about matching the version, and more about issues upgrading composer itself since PHP can't reload classes it's already loaded: composer/composer#11672

I wonder if this should also be improved upstream. Perhaps composer should never install vendor/bin/composer, or if it needs to it should be something like vendor/bin/composer.internal.

I worry that adding too much detail in places like this just makes it so people can't make it through the docs, or find the right thing when they need it.

I think if people use Composer in such an advanced way, they'll know how to change something in $PATH.
I'll remove this example from the docs because it's unlikely to be found.

rfay · 2024-11-06T18:53:08Z

pkg/ddevapp/composer.go

 	path, _, err := app.Exec(&ExecOpts{
-		Cmd: "echo $PATH",
+		Cmd: `echo $PATH | sed "s|~|$HOME|g"`,


I wonder if homedir.Expand() could be used successfully here. Probably not, since this is in the context of the container. But transformations like this are notoriously fragile.

(I don't understand what homedir.Expand() replaces the ~ with. It it's $HOME, it's good, I don't think it is.)

This PR is not only about Composer binary override, but also if in the future we have some commands that will work just like ddev composer with PATH.

This entry in PATH=~/bin should really be PATH=$HOME/bin, then we don't need these ugly workarounds.

I'll also look into the PATH problem with subshells while I'm here:

v1.23.5 regression: ddev composer command error when passing JSON or arg with spaces #6628 (comment)

stasadev · 2024-11-06T22:33:04Z

Side note: when you have ~/bin inside a container with:

hooks:
    post-start:
        - exec: mkdir -p ~/bin

and run

ddev ssh
echo $PATH
/home/stas/bin:/home/stas/bin:/var/www/html/vendor/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/var/www/html/bin

It has a second entry for $HOME/bin, that's because of this Debian default:

ddev/containers/ddev-webserver/ddev-webserver-etc-skel/etc/skel/.profile

Lines 19 to 22 in 0a99831

    
           # set PATH so it includes user's private bin if it exists 
        
           if [ -d "$HOME/bin" ] ; then 
        
               PATH="$HOME/bin:$PATH" 
        
           fi

It doesn't check if $HOME/bin is already here.

I decided not to change/fix it.
When you run ddev composer, $PATH has only one $HOME/bin entry, and the second entry only appears inside the container (with ddev ssh).

stasadev · 2024-11-06T22:35:07Z

containers/ddev-webserver/ddev-webserver-base-files/etc/bashrc/commandline-addons.bashrc

-export PATH="~/bin:$PATH:/var/www/html/bin"
+case ":$PATH:" in
+    *":$HOME/bin:"*) ;;
+    *) PATH="$HOME/bin:$PATH" ;;


So the fix is to write $HOME/bin instead of ~/bin just like Debian does.

This also needs a comment explaining what it's doing.

But it's great, and an excellent solution.

rfay · 2024-11-07T00:59:24Z

Rebased for colima/lima problem

rfay

Minor suggestions just to make sure future readers can grok what's going on.

rfay · 2024-11-08T17:25:53Z

containers/ddev-webserver/ddev-webserver-base-files/etc/bash.nointeractive.bashrc

+case ":$PATH:" in
+    *":${DDEV_COMPOSER_ROOT:-/var/www/html}/vendor/bin:"*) ;;
+    *) PATH="${DDEV_COMPOSER_ROOT:-/var/www/html}/vendor/bin:$PATH" ;;
+esac


I think this is a fine solution, but it's really, really cryptic. Please add a comment to both places explaining what it does.

rfay · 2024-11-08T17:27:13Z

containers/ddev-webserver/ddev-webserver-base-files/etc/bashrc/commandline-addons.bashrc

-export PATH="~/bin:$PATH:/var/www/html/bin"
+case ":$PATH:" in
+    *":$HOME/bin:"*) ;;
+    *) PATH="$HOME/bin:$PATH" ;;


This also needs a comment explaining what it's doing.

But it's great, and an excellent solution.

stasadev · 2024-11-08T17:38:46Z

Comments added (without a new image rebuild).

rfay

Thanks!

rfay · 2024-11-08T20:12:47Z

Rebased, fixed versionconstants, needs image push. Could you push the ddev-webserver image or give me perms on your fork?

stasadev requested review from a team as code owners November 5, 2024 14:44

github-actions bot added the bugfix label Nov 5, 2024

stasadev mentioned this pull request Nov 5, 2024

docs: Show how to use a post-start hook to set the composer version #6688

Closed

stasadev force-pushed the 20241105_stasadev_composer_bin branch from e605e9f to 4129261 Compare November 5, 2024 14:47

rfay requested changes Nov 6, 2024

View reviewed changes

stasadev force-pushed the 20241105_stasadev_composer_bin branch from 411afd0 to e754818 Compare November 6, 2024 21:33

stasadev changed the title ~~fix: respect ~/bin for composer commands, for #6627~~ fix: expand ~/bin in PATH and ensure consistency in subshells, for #6627 Nov 6, 2024

github-actions bot added the dependencies Pull requests that update a dependency file label Nov 6, 2024

stasadev commented Nov 6, 2024

View reviewed changes

rfay force-pushed the 20241105_stasadev_composer_bin branch from 2c506da to 10ef45e Compare November 7, 2024 00:59

rfay requested changes Nov 8, 2024

View reviewed changes

rfay approved these changes Nov 8, 2024

View reviewed changes

stasadev added 5 commits November 8, 2024 13:11

fix: respect ~/bin for composer commands, for ddev#6627

eb74292

docs: remove AI generated block

0ec0b65

fix: remove duplicates from subshells [skip ci]

8892a47

docs: remove example for composer symlink

1a77fe7

Add comments explaining what's going on here [skip ci]

7f481cc

rfay force-pushed the 20241105_stasadev_composer_bin branch from f3a56f2 to 7f481cc Compare November 8, 2024 20:12

rfay merged commit 62bf39b into ddev:master Nov 8, 2024
4 of 9 checks passed

rfay deleted the 20241105_stasadev_composer_bin branch November 8, 2024 20:31

stasadev mentioned this pull request Nov 14, 2024

The relative path ‘~/bin’ is included in the PATH environment variable, which is insecure in combination with the -execdir action of find #6722

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: expand `~/bin` in PATH and ensure consistency in subshells, for #6627 #6689

fix: expand `~/bin` in PATH and ensure consistency in subshells, for #6627 #6689

stasadev commented Nov 5, 2024 •

edited

Loading

rfay commented Nov 5, 2024

github-actions bot commented Nov 5, 2024 •

edited

Loading

stasadev commented Nov 5, 2024

rfay commented Nov 5, 2024

stasadev commented Nov 5, 2024 •

edited

Loading

rfay left a comment

rfay Nov 6, 2024

deviantintegral Nov 6, 2024

stasadev Nov 6, 2024

rfay Nov 6, 2024

stasadev Nov 6, 2024

stasadev commented Nov 6, 2024

stasadev Nov 6, 2024 •

edited

Loading

rfay Nov 8, 2024

rfay commented Nov 7, 2024

rfay left a comment

rfay Nov 8, 2024

rfay Nov 8, 2024

stasadev commented Nov 8, 2024

rfay left a comment

rfay commented Nov 8, 2024

fix: expand ~/bin in PATH and ensure consistency in subshells, for #6627 #6689

fix: expand ~/bin in PATH and ensure consistency in subshells, for #6627 #6689

Conversation

stasadev commented Nov 5, 2024 • edited Loading

The Issue

How This PR Solves The Issue

Manual Testing Instructions

Automated Testing Overview

Release/Deployment Notes

rfay commented Nov 5, 2024

github-actions bot commented Nov 5, 2024 • edited Loading

stasadev commented Nov 5, 2024

rfay commented Nov 5, 2024

stasadev commented Nov 5, 2024 • edited Loading

rfay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stasadev commented Nov 6, 2024

stasadev Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfay commented Nov 7, 2024

rfay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stasadev commented Nov 8, 2024

rfay left a comment

Choose a reason for hiding this comment

rfay commented Nov 8, 2024

fix: expand `~/bin` in PATH and ensure consistency in subshells, for #6627 #6689

fix: expand `~/bin` in PATH and ensure consistency in subshells, for #6627 #6689

stasadev commented Nov 5, 2024 •

edited

Loading

github-actions bot commented Nov 5, 2024 •

edited

Loading

stasadev commented Nov 5, 2024 •

edited

Loading

stasadev Nov 6, 2024 •

edited

Loading