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

exec-env improvements #80

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

scop
Copy link

@scop scop commented Apr 23, 2022

See individual commit messages for details on each change.

@missingcharacter
Copy link
Contributor

Checking if GOROOT and GOPATH should be more than enough, there is no need to add unset or add 2 environment variables (ASDF_GOLANG_DISABLE_GOROOT, ASDF_GOLANG_DISABLE_GOPATH

@missingcharacter
Copy link
Contributor

missingcharacter commented Apr 23, 2022

I just finished reading asdf-community/asdf-direnv#149, I think GOPATH should not be changed if it was already set since GOPATH is normally a developer's workspace.

but you are right about unsetting/setting GOROOT depending on the switch between versions

@scop
Copy link
Author

scop commented Apr 25, 2022

I agree changing is more crucial for GOROOT if it's set.

For GOPATH, one approach would be to change if the already set value looks like something asdf-golang has set it to earlier, otherwise leave alone.

Anyway, I can revise this PR once I know what behavior the maintainers here prefer.

Personally, I would drop touching both GOROOT and GOPATH altogether, no matter if set or not. That'd be a backwards incompatible change though -- therefore the ASDF_GOLANG_DISABLE_* variables, to make the breaking change opt-in.

@kennyp
Copy link
Member

kennyp commented Dec 17, 2022

Thanks for the PR @scop! I've got the next two weeks off, so I'll spend some time digging into this one.

scop added 2 commits January 1, 2023 21:46
Otherwise they -- especially GOROOT -- may end up pointing to a
completely wrong one.
Otherwise, existing values from the environment may continue to point to
a wrong version.

Refs asdf-community/asdf-direnv#149
@scop scop force-pushed the feat/exec-env-improvements branch from 168b974 to aa82aae Compare January 1, 2023 19:49
@scop
Copy link
Author

scop commented Jan 1, 2023

Rebased to resolve conflicts, cleaned up indentation some.

…TH} is 1

Setting these variables is not really required for things to work, and
some setups explicitly prefer them not to be touched.

Modeled after the similar options in goenv.
https://github.com/syndbg/goenv/blob/d4e2dd79b83496dbf9474e31dbdb8d7eb8bb0261/ENVIRONMENT_VARIABLES.md
@scop scop force-pushed the feat/exec-env-improvements branch from aa82aae to 91cd860 Compare January 1, 2023 19:52
@scop
Copy link
Author

scop commented Jul 17, 2023

Anything I can do to help with this?

@darthShadow
Copy link

Hi,

Is there anything that can be done to get this merged? Is there anything missing or broken here, apart from the conflicts due to the age of the PR?

@darthShadow
Copy link

Just to clarify my reason for bumping this, my custom GOPATH is always overwritten right now by the GOPATH in the exec-env script.

This seems to be because GOPATH is never passed through by asdf when calling the exec-env script so it falls through to the unset condition.

References:

Prepare the environment before executing the shims for the binaries for the tool.

**Environment Variables available to script**

- `ASDF_INSTALL_TYPE`: `version` or `ref`
- `ASDF_INSTALL_VERSION`:
  - Full version number if `ASDF_INSTALL_TYPE=version`.
  - Git ref (tag/commit/branch) if `ASDF_INSTALL_TYPE=ref`.
- `ASDF_INSTALL_PATH`: The path to where the tool _has been_, or _should be_ installed.

@missingcharacter
Copy link
Contributor

@darthShadow, was this happening with asdf versions of 0.15.0?

I ask because asdf version 0.16.0 and above overlooked a bunch of behaviors/features we "got for free" by using bash, and now, with the rewrite in go, asdf needs to acknowledge and finally communicate if they'll implement them or ask maintainers of plugins to "deal with it".

@darthShadow
Copy link

Can't really say for sure.

Theoretically, I would say yes, because bash may have been passing through the variables unless they were cleared intentionally, but golang requires them to be explicitly declared for any shell-command executions.

However, I recently moved from Goland to Cursor, and it requires a couple of tools to be installed separately. That switch led me to this issue since the installs were going to a different path than expected and required a reinstall on every restart of Cursor.

@missingcharacter
Copy link
Contributor

@darthShadow you can still downgrade to asdf version 0.15.0 and below if you'd like to confirm, this can help @asdf-community/asdf-golang

@darthShadow
Copy link

Yep, can confirm v0.15 is passing through all environment variables and v0.16 is only passing the specified subset.

@missingcharacter
Copy link
Contributor

@darthShadow I believe your issue will be fixed when asdf-vm/asdf#1899 is merged. There may be other places where asdf needs to pass variables to go's syscall.Exec and like I said before:

asdf needs to acknowledge and finally communicate if they'll implement them or ask maintainers of plugins to "deal with it".

@darthShadow
Copy link

I don't think that specific PR will help? That seems to be only for plugin updates and doesn't pass through GOPATH.

I would guess this needs a separate PR upstream that either allows all environment variables to pass through as before or allows a plugin to specify which ones it is interested in. The second option is probably cleaner but will require changes in all plugins relying on the first option.

@darthShadow
Copy link

Upstream bug report: asdf-vm/asdf#1924

@missingcharacter
Copy link
Contributor

Thanks @darthShadow

@scop
Copy link
Author

scop commented Feb 9, 2025

JFYI, I do not intend to work further on this or resolve conflicts etc, as I'm no longer using asdf (have switched to mise). I suggest someone who does take this over.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants