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: Do not return full env map after downloading releases #817

Merged
merged 1 commit into from
Aug 23, 2024

Conversation

StrongMonkey
Copy link
Contributor

After downloading gptscript release, we return a full map of current environment variables and this will eventuall get saved in to done.file, which will be picked up and saved as environment variable on the next run. This is causing weird issues like gptscript-ai/desktop#253. The original purpose of adding env variable to done.file is probably related to only append PATH, as we can see the logic here:

newEnv := runtimeEnv.AppendPath(env, binPath)
. But this is not needed when downloading releases because it is always downloaded in known location.

Signed-off-by: Daishan Peng <daishan@acorn.io>
Copy link
Contributor

@iwilltry42 iwilltry42 left a comment

Choose a reason for hiding this comment

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

The go runtime is the only one even implementing the Binary() interface func, so I guess we could completely remove the env param, couldn't we?
Especially given your argument, that it doesn't make sense 😁

@StrongMonkey
Copy link
Contributor Author

yes, I don't actually want to change too much scope just in case though.

@cjellick
Copy link
Contributor

You dont think we're passing down anything else useful in these env vars?

@StrongMonkey
Copy link
Contributor Author

You dont think we're passing down anything else useful in these env vars?

I looked at other code and the only thing we are doing is to pass PATH environment so that the binary we built is baked into the PATH. Which i don't think we needed for downloading releases. So I think this change should be good and darren did not include this intentionally.

@StrongMonkey StrongMonkey merged commit 3f2a8ee into gptscript-ai:main Aug 23, 2024
10 checks passed
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.

5 participants