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

Properly use XDG specs #2592

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

Conversation

hauleth
Copy link

@hauleth hauleth commented Jun 30, 2021

Before setting XDG_{CONFIG,CACHE}_HOME didn't changed Rebar3 behaviour which would still, forcefully, use $HOME/.config and $HOME/.cache respectively. Now it will properly read these environment variables and work accordingly.

src/rebar_dir.erl Outdated Show resolved Hide resolved
@tsloughter
Copy link
Collaborator

Oh great, I'd been waiting until we supported only the versions of OTP and above that included the XDG stuff to bother with this, and this was added in 19, so good to go!

@tsloughter
Copy link
Collaborator

Except for all the tests failing :)

Before setting `XDG_{CONFIG,CACHE}_HOME` didn't changed Rebar3 behaviour
which would still, forcefully, use `$HOME/.config` and `$HOME/.cache`
respectively. Now it will properly read these environment variables and
work accordingly.
@hauleth hauleth force-pushed the fix/properly-use-xdg-paths branch from 37fdf32 to 49e6d13 Compare June 30, 2021 18:48
@hauleth
Copy link
Author

hauleth commented Jun 30, 2021

@tsloughter you need to reprove workflows.

@tsloughter
Copy link
Collaborator

I've got to lookinto stopping it from making me manually allow those..

@hauleth
Copy link
Author

hauleth commented Jun 30, 2021

@tsloughter after this will merge you will no longer need to manually allow these for me, but you still will need that for other first-time contributors as it is security measure against malicious 3rd parties that used GitHub Actions for crypto mining and other dubious stuff from the GitHub and maintainers perspective.

@@ -133,16 +133,22 @@ home_dir() ->
{ok, [[Home]]} = init:get_argument(home),
Home.

basedir(Type) ->
filename:basedir(Type, "rebar3", #{os => linux}).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, hm, if we are going to do this we probably shouldn't hardcode linux. It may change where stuff goes in windows and osx but probably better to be right than continue being wrong? But not sure...

@ferd thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

I am on macOS and I prefer my development tooling to use XDG rather than macOS practices ;)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This may unfortunately blow out things in the global rebar.config file and existing hex auth and be considered backwards incompatible in very surprising ways.

If we're to support the new stuff, it should ideally come with supporting both at once with a warning or information message asking the user to move content, which they may hate if they have to use various rebar3 versions for various OTP support (or when building with mix, which bundles older ones).

Forcing the linux path likely keeps things compatible with the current mechanism on all operating systems though, since it's what we forced everywhere (would have to double check windows)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yea. I suppose we can wait for rebar4.

Copy link
Author

Choose a reason for hiding this comment

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

@ferd in what way it can blow things? It would only change behaviour if user have set XDG_CONFIG_HOME.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Using it with the linux path everywhere should remain the same although there's a check needed for windows because I don't know if they're fully equivalent. Following the basedir fully would break stuff.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ferd what check for windows are you referring to?

I think this PR is good to merge except for a question I have on line 151.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we're fine if we keep forcing the linux format. I just double-checked, the thing that worried me is that there could be a conversion issue between what was considered the home_dir() in windows as of today vs. how the Linux path would translate.

I did a check on my old win10 machine and it seems to be identical and good to go.


rebar_config_dir(State) ->
case os:getenv("REBAR_GLOBAL_CONFIG_DIR") of
false ->
rebar_state:get(State, global_rebar_dir, home_dir());
rebar_state:get(State, global_rebar_dir, undefined);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this now undefined?

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.

4 participants