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

Adds godot symlink to OS' PATH, auto-detect default SHELL on UNIX systems #54

Merged
merged 14 commits into from
Jan 13, 2024

Conversation

edassis
Copy link
Contributor

@edassis edassis commented Dec 12, 2023

Hi,

I have been messing around with godotenv for some weeks now, and I really liked it. I'm mainly using it to manage my godot installations.

My major complaint is that I can't call the new godot install through the terminal easily. The installation process gives me the GODOT env-var, but not exports the symlink in the system PATH nor provides some guidance of how to do it.

This PR addresses this and another improvements:

Mainly touches in godot env setup and godot install commands.

  • Godot binary path now is ../godotenv/godot/bin/godot instead of ../godotenv/godot/bin;

    GODOT env was updated accordingly.

  • Exports godotenv's godot bin folder in user's PATH on all OSes;

    Now it's possible to call godot by just typing it on the terminal.

  • Env var changes on Windows occur on USER environment now, instead of SYSTEM;

    This change tries to make env-var setup more concise across OSes and minimizes the need for an elevated cmd shell. SystemEnvironmentVariableClient class was renamed to EnvironmentVariableClient. Some methods name has been changed:
    SetEnvSetUserEnv
    GetEnvGetUserEnv
    References in documentation citing "system environment ..." was updated to "user environment ...".

  • FileClient.AddLinesToFileIfNotPresent() now writes to the end of the file;

    This way, new var exports have priority over older ones.

  • Tries to detect the user's default shell automatic;

    People changes the default shell on Linux frequently. So this tries to make a more sensible decision.
    To keep things simple and not break godotenv's shell commands, it is assumed that the user's shell is POSIX-compliant (bash, zsh, sh).
    Two new functions have been added in EnvironmentVariableClient.GetDefaultShell(), and
    EnvironmentVariableClient.IsShellSupported().
    I think it would be great to call these functions in the program initialization (before running any command) to detect the user's default shell and validate it. But I didn't figure out a good way to do it.
    This should resolve the issue [FEATURE] Support for other shells #32.

  • Tests.
    Added some tests for EnvironmentVariableClient and GodotRepository.

I did manual testing on Mac M1 13.6.1 (zsh), Ubuntu 22 (zsh and bash) and Windows 11 (PowerShell).

Let me know what you guys think. In case necessary, I can remove or split the changes in another PR.


EnvironmentVariableClient's SetUserEnv() and AppendToUserEnv() are declared as async function, but their routines are totally synchronous at the moment. So I think we can change their definitions.

And, the other way around, GetUserEnv() and GetDefaultShell() are synchronous but could be made asynchronous.


Some pictures:

  • GODOT env:
    GODOT env

  • PATH updated:
    PATH updated

  • Calling from terminal:
    Calling from terminal

  • User's shell not supported behavior:
    User's shell not supported

@jolexxa
Copy link
Member

jolexxa commented Dec 14, 2023

Thanks for taking a look and working on all this.

Let me know when this is ready for a closer look.

This makes the Windows code routine more similar to the UNIX one.

- Adds logic to append new path to an existing variable
(windows only currently);
- Updates documentation about env-var;
Get value from shell, as this will validate if the variable is being
overwritten in some place. Making the information more concise with what
the user is expecting.

- 'AppendToUserEnv' append to the start to give the new value the
highest priority.
@edassis edassis force-pushed the fix/env_var branch 2 times, most recently from 1069fc1 to 32a4ab7 Compare December 16, 2023 18:40
@edassis edassis force-pushed the fix/env_var branch 3 times, most recently from a7dc526 to 635c808 Compare December 21, 2023 22:56
@jolexxa jolexxa marked this pull request as ready for review December 22, 2023 00:36
@jolexxa jolexxa marked this pull request as draft December 22, 2023 00:40
@edassis edassis force-pushed the fix/env_var branch 2 times, most recently from 8cd77de to a766505 Compare December 26, 2023 22:28
@edassis edassis force-pushed the fix/env_var branch 5 times, most recently from d2c0db7 to 9c5a5bd Compare January 2, 2024 19:43
@jolexxa jolexxa marked this pull request as ready for review January 4, 2024 14:55
@jolexxa jolexxa marked this pull request as draft January 4, 2024 14:55
@jolexxa
Copy link
Member

jolexxa commented Jan 4, 2024

Let me know if this is ready and I'll take a closer look. (I keep accidentally pressing the "ready for review" button for some reason)

- Creates 'EnvironmentClient' to wrap System.Environment methods.
- Create specific cases for 'OSX' and 'Linux' in PlatformFact.
@edassis edassis marked this pull request as ready for review January 6, 2024 18:46
@edassis
Copy link
Contributor Author

edassis commented Jan 6, 2024

I think this PR is in good shape for review now! Let me know what you think and if I need to change something.

@jolexxa
Copy link
Member

jolexxa commented Jan 7, 2024

Thank you for all your hard work, things are looking great! I left a few comments here and there, nothing major.

@edassis edassis requested a review from jolexxa January 8, 2024 00:59
@edassis
Copy link
Contributor Author

edassis commented Jan 8, 2024

@definitelyokay, I'm not seeing any pending comments or conversation for me to resolve. Maybe I'm doing something wrong? I don't know 😅.

Copy link
Member

@jolexxa jolexxa left a comment

Choose a reason for hiding this comment

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

ah, forgot to actually submit my review 😆

README.md Outdated Show resolved Hide resolved
GodotEnv/src/common/clients/EnvironmentVariableClient.cs Outdated Show resolved Hide resolved
GodotEnv/src/common/clients/EnvironmentVariableClient.cs Outdated Show resolved Hide resolved
GodotEnv/src/common/clients/EnvironmentVariableClient.cs Outdated Show resolved Hide resolved
@jolexxa
Copy link
Member

jolexxa commented Jan 9, 2024

Thank you so much for all your hard work on this. Just the one little comment, it's looking really good!

@edassis
Copy link
Contributor Author

edassis commented Jan 10, 2024

I'm making a last round of tests.

- Better logic to define default shell when the user's one is invalid (MacOS -> zsh, Linux -> bash, Windows -> empty).
- More descriptive code comments
Make new cases to validate shell defaulting when user's shell is valid/invalid.
@edassis
Copy link
Contributor Author

edassis commented Jan 10, 2024

Seems to be working as expected :).

.gitignore Outdated Show resolved Hide resolved
@jolexxa jolexxa merged commit 84d949a into chickensoft-games:main Jan 13, 2024
7 checks passed
@edassis edassis deleted the fix/env_var branch January 13, 2024 15:04
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.

2 participants