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.

Sign up for GitHub

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

Jump to bottom

Properly use XDG specs #2592

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

Properly use XDG specs #2592

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions src/rebar_dir.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Replace my "may" with "will". https://erlang.org/doc/man/filename.html#basedir-3

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.


%% @doc returns the directory where the global configuration files for rebar3
%% may be stored.
-spec global_config_dir(rebar_state:t()) -> file:filename_all().
global_config_dir(State) ->
filename:join([rebar_config_dir(State), ".config", "rebar3"]).
case rebar_config_dir(State) of
undefined -> basedir(user_config);
Path -> filename:join([Path, ".config", "rebar3"])
end.

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?

ConfDir ->
ConfDir
end.
Expand All @@ -155,14 +161,12 @@ global_config(State) ->
%% @doc returns the default path of the global rebar.config file
-spec global_config() -> file:filename_all().
global_config() ->
Home = home_dir(),
filename:join([Home, ".config", "rebar3", "rebar.config"]).
filename:join([basedir(user_config), "rebar.config"]).

%% @doc returns the location for the global cache directory
-spec global_cache_dir(rebar_dict()) -> file:filename_all().
global_cache_dir(Opts) ->
Home = home_dir(),
rebar_opts:get(Opts, global_rebar_dir, filename:join([Home, ".cache", "rebar3"])).
rebar_opts:get(Opts, global_rebar_dir, basedir(user_cache)).

%% @doc appends the cache directory to the path passed to this function.
-spec local_cache_dir(file:filename_all()) -> file:filename_all().
Expand Down