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 setting INSTALL_DIR_FOR_OTP #141

Merged
merged 1 commit into from
Sep 16, 2022
Merged

Fix setting INSTALL_DIR_FOR_OTP #141

merged 1 commit into from
Sep 16, 2022

Conversation

@wojtekmach
Copy link
Collaborator Author

On Windows we have this env vars output:

D:\a\_temp/.setup-beam/elixir
D:\a\_temp/.setup-beam/gleam
D:\a\_temp\.setup-beam\otp
D:\a\_temp/.setup-beam/rebar3

notice it's mix of / and \ everywhere besides now OTP. Happy to make OTP like the others or change others to use just \. Happy to do it here or in a separate PR.

@paulo-ferraz-oliveira
Copy link
Collaborator

notice it's mix of / and \ everywhere besides now OTP. Happy to make OTP like the others or change others to use just . Happy to do it here or in a separate PR.

If \ is more like Windows users are used to, go for it 😄. I used it (/), during development, to ease diff.s between the *nix and Windows scripts, which is why you'll find similar variable names, and whatnot. (but the scripts didn't grow much, so that's probably manageable, in any case).

On the other hand, should we add some simple tests to this to make sure the env. variables are set?

I'm approving, in any case, to ease your merge, since I believe that it's good to ship even as-is.

@wojtekmach wojtekmach merged commit d85df62 into main Sep 16, 2022
@wojtekmach wojtekmach deleted the wm-fix-env branch September 16, 2022 08:29
@wojtekmach
Copy link
Collaborator Author

Thank you! I agree we should ship it as is as it already broke one project. We can always improve it down the road.

@wojtekmach
Copy link
Collaborator Author

Agreed about testing this. We already have a job that prints these env vars so I think an easy improvement is this:

- echo $A
+ [ -n "$A" ] && echo $A

I'll send a patch soon!

@paulo-ferraz-oliveira
Copy link
Collaborator

Good stuff, @wojtekmach.

@paulo-ferraz-oliveira
Copy link
Collaborator

I'll release this as 1.13.1. The next PR can go later...

@paulo-ferraz-oliveira
Copy link
Collaborator

@wojtekmach
Copy link
Collaborator Author

Thanks!

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.

None yet

2 participants