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(cli): do not create bin folder within DENO_INSTALL_ROOT #27459

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mahtaran
Copy link

As can be seen in the documentation, DENO_INSTALL_ROOT is meant to be the folder wherein Deno will install executables, with the default being $HOME/.deno/bin.

However, the current behaviour of the install and uninstall commands, if DENO_INSTALL_ROOT is set, is to still append "bin" to the path. This leads to executables being installed in a folder like $HOME/.deno/bin/bin, which is obviously not the intended behaviour.

This commit removes this appending. Do note that this is a breaking change in the behaviour of the --root flag, as previously it referred to a folder wherein the bin folder was located, but now it refers to the bin folder itself. For example: deno install --root /foo/bar test.ts would previously install the executable in /foo/bar/bin/test, but now it will install it in /foo/bar/test.

This would also most certainly break many third-party tools that expect the current behaviour. See for example:

Closes #26442

Please do note that, as I am currently on a family visit out of the country, I currently only have my poor little laptop. As such, I do not have the storage capacity to build the project or run the test suite, so THIS CODE IS UNTESTED. If someone else could test it for me, that'd be grand, and otherwise I'll do it when I'm back home in a couple of weeks.

@CLAassistant
Copy link

CLAassistant commented Dec 24, 2024

CLA assistant check
All committers have signed the CLA.

@mahtaran
Copy link
Author

mahtaran commented Dec 24, 2024

I seem to have completely missed that @dovakin0007 already submitted a fix in #26446, apologies! I do believe this PR is more 'correct' as it modifies the behaviour to be what the documentation says it is, however Gowtham's PR will cause noticeably less (but likely still some) breaking of third-party tools and existing set-ups. I'll leave the choice on what's best up to the maintainers.

@mahtaran mahtaran force-pushed the fix/install-root branch 2 times, most recently from ba289ed to 8f1e194 Compare December 25, 2024 22:39
@mahtaran mahtaran changed the title 💥 Correct the behaviour of DENO_INSTALL_ROOT within (un)install commands fix(cli): correct the behaviour of DENO_INSTALL_ROOT within (un)install commands Dec 25, 2024
@mahtaran mahtaran changed the title fix(cli): correct the behaviour of DENO_INSTALL_ROOT within (un)install commands fix(cli): do not create bin folder within DENO_INSTALL_ROOT Dec 25, 2024
@mahtaran mahtaran force-pushed the fix/install-root branch 5 times, most recently from 468d890 to d763edb Compare December 26, 2024 02:53
As can be seen in the documentation, `DENO_INSTALL_ROOT` is meant to be the folder wherein Deno will install executables, with the default being `$HOME/.deno/bin`.

However, the current behaviour of the `install` and `uninstall` commands, if `DENO_INSTALL_ROOT` is set, is to still append "bin" to the path. This leads to executables being installed in a folder like `$HOME/.deno/bin/bin`, which is obviously not the intended behaviour.

This commit removes this appending. Do note that this is a breaking change in the behaviour of the `--root` flag, as previously it referred to a folder wherein the `bin` folder was located, but now it refers to the `bin` folder itself. For example: `deno install --root /foo/bar test.ts` would previously install the executable in `/foo/bar/bin/test`, but now it will install it in `/foo/bar/test`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent DENO_INSTALL_ROOT behaviour
2 participants