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

[#177] publish does not include docs #180

Merged
merged 3 commits into from
Aug 31, 2020
Merged

[#177] publish does not include docs #180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0d50d63
[#177] Code formatting
jfacorro Aug 17, 2020
2b7711d
[#177] Make sure publishing docs fails if directory doesn't exist
jfacorro Aug 17, 2020
2114542
[#177] Remove edoc command as a dependency of docs command
jfacorro Aug 17, 2020
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: 11 additions & 5 deletions src/rebar3_hex.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
-module(rebar3_hex).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whitespace only changes.


-export([init/1, gather_opts/2, get_required/2, task_args/1, repo_opt/0, help_opt/0]).
-export([ init/1
, gather_opts/2
, get_required/2
, task_args/1
, repo_opt/0
, help_opt/0
]).

init(State) ->
lists:foldl(fun provider_init/2, {ok, State}, [rebar3_hex_user,
Expand All @@ -17,13 +23,13 @@ init(State) ->
provider_init(Module, {ok, State}) ->
Module:init(State).

gather_opts(Targets, State) ->
gather_opts(Targets, State) ->
{Args, _} = rebar_state:command_parsed_args(State),
lists:foldl(fun(T, Acc) ->
lists:foldl(fun(T, Acc) ->
case proplists:get_value(T, Args, undefined) of
undefined ->
undefined ->
Acc;
V ->
V ->
maps:put(T, V, Acc)
end
end, #{}, Targets).
Expand Down
26 changes: 13 additions & 13 deletions src/rebar3_hex_client.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,19 @@
-module(rebar3_hex_client).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whitespace only changes.


-export([ is_success/1
, key_add/3
, key_get/2
, key_delete/2
, key_delete_all/1
, key_list/1
, publish/3
, publish_docs/4
, delete_docs/3
, test_key/2
, member_of/1
, pretty_print_status/1
, pretty_print_errors/1]).
, key_add/3
, key_get/2
, key_delete/2
, key_delete_all/1
, key_list/1
, publish/3
, publish_docs/4
, delete_docs/3
, test_key/2
, member_of/1
, pretty_print_status/1
, pretty_print_errors/1
]).

-include("rebar3_hex.hrl").

Expand All @@ -30,7 +31,6 @@ key_get(HexConfig, <<KeyName/binary>>) ->
key_get(HexConfig, KeyName) ->
key_get(HexConfig, to_binary(KeyName)).


member_of(HexConfig) ->
Res = hex_api_organization:list(HexConfig),
response(Res).
Expand Down
49 changes: 24 additions & 25 deletions src/rebar3_hex_config.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
-module(rebar3_hex_config).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly whitespace only changes.


-export([api_key_name/1
, api_key_name/2
, repos_key_name/0
, org_key_name/2
, parent_repos/1
, hex_config/2
, hex_config_write/1
, hex_config_read/1
, repo/1
, update_auth_config/2
-export([ api_key_name/1
, api_key_name/2
, repos_key_name/0
, org_key_name/2
, parent_repos/1
, hex_config/2
, hex_config_write/1
, hex_config_read/1
, repo/1
, update_auth_config/2
]).

-include("rebar3_hex.hrl").
Expand All @@ -33,6 +33,7 @@ org_key_name(Key, Org) ->
Prefix = key_name_prefix(Key),
key_name(Prefix, <<"-repository-">>, Org).

-spec hostname() -> binary().
hostname() ->
{ok, Name} = inet:gethostname(),
list_to_binary(Name).
Expand All @@ -55,12 +56,10 @@ repo(State) ->
#{repos := Repos} = rebar_resource_v2:find_resource_state(pkg, Resources),
case proplists:get_value(repo, Args, undefined) of
undefined ->
DefaultBinName = rebar_utils:to_binary(?DEFAULT_HEX_REPO),
Res = lists:filter(fun(R) -> maps:get(name, R) =/= DefaultBinName end,
Repos),
Res = [R || R <- Repos, maps:get(name, R) =/= ?DEFAULT_HEX_REPO],
case Res of
[] ->
case rebar_hex_repos:get_repo_config(rebar_utils:to_binary(?DEFAULT_HEX_REPO), Repos) of
case rebar_hex_repos:get_repo_config(?DEFAULT_HEX_REPO, Repos) of
{ok, Repo} ->
{ok, Repo};
_ ->
Expand Down Expand Up @@ -90,12 +89,13 @@ repo(State, RepoName) ->
end.


-define(ENV_VARS, [
{"HEX_API_KEY", {api_key, {string, undefined}}},
{"HEX_API_URL", {api_url, {string, undefined}}},
{"HEX_UNSAFE_REGISTRY", {repo_verify, {boolean, false}}},
{"HEX_NO_VERIFY_REPO_ORIGIN", {repo_verify_origin, {boolean, true}}}
]).
-define( ENV_VARS
, [ {"HEX_API_KEY", {api_key, {string, undefined}}}
, {"HEX_API_URL", {api_url, {string, undefined}}}
, {"HEX_UNSAFE_REGISTRY", {repo_verify, {boolean, false}}}
, {"HEX_NO_VERIFY_REPO_ORIGIN", {repo_verify_origin, {boolean, true}}}
]
).

merge_with_env(Repo) ->
lists:foldl(fun({EnvName, {Key, _} = Default}, Acc) ->
Expand Down Expand Up @@ -152,11 +152,10 @@ get_repo(BinaryName, Repos) ->
{error,{rebar_hex_repos,{repo_not_found,BinaryName}}} -> undefined
end.

hex_config(Repo, Op) ->
case Op of
read -> hex_config_read(Repo);
write -> hex_config_write(Repo)
end.
hex_config(Repo, read) ->
hex_config_read(Repo);
hex_config(Repo, write) ->
hex_config_write(Repo).

hex_config_write(#{api_key := Key} = HexConfig) when is_binary(Key) ->
{ok, HexConfig};
Expand Down
97 changes: 65 additions & 32 deletions src/rebar3_hex_docs.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@
-include("rebar3_hex.hrl").

-define(PROVIDER, docs).
-define(DEPS, [{default, edoc}]).
-define(DEPS, [{default, lock}]).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the dependency on edoc addresses #57 in a way.


-define(ENDPOINT, "packages").
-define(DEFAULT_DOC_DIR, "doc").

%% ===================================================================
%% Public API
Expand Down Expand Up @@ -46,29 +47,50 @@ do(State) ->
?PRV_ERROR(Reason)
end.

%% The following function is made available and exported for publishing docs via the main the publish command.
%% @doc Publish documentation directory to repository
%%
%% This following function is exported for publishing docs via the
%% main the publish command.
-spec publish(rebar_app_info:t(), rebar_state:t(), map()) ->
{ok, rebar_state:t()}.
publish(App, State, Repo) ->
handle_command(App, State, Repo).

publish_apps(Apps, State) ->
lists:foldl(fun(App, {ok, StateAcc}) ->
case handle_command(App, StateAcc) of
{ok, _StateAcc} ->
{ok, StateAcc};
Err ->
throw(Err)
end
-spec format_error(any()) -> iolist().
format_error(bad_command) ->
"Invalid command and/or options provided";
format_error({publish, {unauthorized, _Res}}) ->
"Error publishing : Not authorized";
format_error({publish, {not_found, _Res}}) ->
"Error publishing : Package or Package Version not found";
format_error({revert, {unauthorized, _Res}}) ->
"Error reverting docs : Not authorized";
format_error({revert, {not_found, _Res}}) ->
"Error reverting docs : Package or Package Version not found";
format_error(Reason) ->
rebar3_hex_error:format_error(Reason).

end, {ok, State}, Apps).
%% ===================================================================
%% Internal Functions
%% ===================================================================

publish_apps(Apps, State) ->
lists:foldl(fun(App, {ok, StateAcc}) ->
case handle_command(App, StateAcc) of
{ok, _StateAcc} ->
{ok, StateAcc};
Err ->
throw(Err)
end
end, {ok, State}, Apps).

handle_command(App, State) ->
{ok, Repo} = rebar3_hex_config:repo(State),
handle_command(App, State, Repo).

handle_command(App, State, Repo) ->
{Args, _} = rebar_state:command_parsed_args(State),
Revert = proplists:get_value(revert, Args, undefined),
case Revert of
case proplists:get_value(revert, Args, undefined) of
undefined ->
do_publish(App, State, Repo);
Vsn ->
Expand All @@ -77,15 +99,17 @@ handle_command(App, State, Repo) ->

do_publish(App, State, Repo) ->
AppDir = rebar_app_info:dir(App),
Files = rebar3_hex_file:expand_paths(["doc"], AppDir),
DocDir = resolve_doc_dir(App),
assert_doc_dir(filename:join(AppDir, DocDir)),
Files = rebar3_hex_file:expand_paths([DocDir], AppDir),
AppDetails = rebar_app_info:app_details(App),
Name = binary_to_list(rebar_app_info:name(App)),
PkgName = rebar_utils:to_list(proplists:get_value(pkg_name, AppDetails, Name)),
OriginalVsn = rebar_app_info:original_vsn(App),
Vsn = rebar_utils:vcs_vsn(App, OriginalVsn, State),

Tarball = PkgName ++ "-" ++ vsn_string(Vsn) ++ "-docs.tar.gz",
ok = erl_tar:create(Tarball, file_list(Files), [compressed]),
ok = erl_tar:create(Tarball, file_list(Files, DocDir), [compressed]),
{ok, Tar} = file:read_file(Tarball),
file:delete(Tarball),

Expand Down Expand Up @@ -118,23 +142,32 @@ do_revert(App, State, Repo, Vsn) ->
?PRV_ERROR({revert, Reason})
end.

file_list(Files) ->
[{drop_path(ShortName, ["doc"]), FullName} || {ShortName, FullName} <- Files].
%% @doc Returns the directory were docs are to be found
%%
%% The priority for resolution is the following:
%% 1. `doc' entry in the application's `*.app.src'.
%% 2. `dir' entry specified in `edoc_opts'.
%% 3. `"doc"' fallback default value.
-spec resolve_doc_dir(rebar_app_info:t()) -> string().
resolve_doc_dir(AppInfo) ->
AppOpts = rebar_app_info:opts(AppInfo),
EdocOpts = rebar_opts:get(AppOpts, edoc_opts, []),
AppDetails = rebar_app_info:app_details(AppInfo),
Dir = proplists:get_value(dir, EdocOpts, ?DEFAULT_DOC_DIR),
proplists:get_value(doc, AppDetails, Dir).

-spec assert_doc_dir(string()) -> true.
assert_doc_dir(DocDir) ->
filelib:is_dir(DocDir) orelse
rebar_api:abort( "Docs were not published since they "
Copy link
Contributor Author

@jfacorro jfacorro Aug 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Since the publish command calls the rebar3_hex_docs:publish/3 function once it has already succeeded publishing the package itself, I though it would be better to use rebar_api:abort/2 for showing the user the error and aborting the execution. This will only abort the docs publishing and not the package publishing.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I wonder if we actually want to bail on the publish 🤔 Like, if you're expecting it all in one go, then I could see where we might want to bail on the package publishing too, but I don't have strong feelings about it either.

Copy link
Member

@starbelly starbelly Aug 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I thought about this more and I really think we should strive for parity with hex (mix hex) here. Specifically, calling rebar3 hex publish assumes you want to publish both, we should check that docs exist before we attempt to publish anything, if they do not, bail and give the user a message.

We should also provide switches so inform publish to only publish the package or only publish the docs. --package and --docs.

The message I would expect in the case that we see no docs and bail would be something like what you have + If you really only want to publish the package without docs use the --package switch

Thoughts?

Copy link
Contributor Author

@jfacorro jfacorro Aug 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think that is a good idea, but I think the discussion around the details of implementing this new approach (i.e. publish = pkg+docs plus --package and --docs flags) deserves its own issue and separate PR. There might be many details that need to be discussed, for example, it would be nice to show the docs directory that will be used when listing the details on what is going to be published.

Since this PR address the bug in the current behaviour discussed in #180 I would like to get it merged to avoid the unexpected results we get today.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To be clear, since I'm missing some context, we are running the edoc provider before publishing, right? If not we should have publish depending on or calling the edoc provider from rebar3 and then doing the publish.

If there aren't any docs even though it ran edoc then it should just continue on I would think, without erroring. Obviously will be an issue of someone overriding where edoc outputs shit, but whatever, can deal with that later.

Copy link
Contributor Author

@jfacorro jfacorro Aug 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The publish command currently also publishes the docs by calling the internal function in rebar3_hex_docs:publish3. It is in this situation that the published docs are empty.

The docs command depends on {default, edoc}. This PR removes that dependency to allow users to rely on other tools to generate documentation.

There is more context in the related issue #177.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah ok. Yea, I guess that function should call the edoc provider instead of having the provider use a dependency on it.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'm cool with merging this now per what @jfacorro points out, i.e., it fixes a very confusing issue.

"couldn't be found in '~s'. "
"Please build the docs and then run "
"`rebar3 hex docs` to publish them."
, [DocDir]
).

file_list(Files, DocDir) ->
[{drop_path(ShortName, [DocDir]), FullName} || {ShortName, FullName} <- Files].

drop_path(File, Path) ->
filename:join(filename:split(File) -- Path).

-spec format_error(any()) -> iolist().
format_error(bad_command) ->
"Invalid command and/or options provided";
format_error({publish, {unauthorized, _Res}}) ->
"Error publishing : Not authorized";
format_error({publish, {not_found, _Res}}) ->
"Error publishing : Package or Package Version not found";
format_error({revert, {unauthorized, _Res}}) ->
"Error reverting docs : Not authorized";
format_error({revert, {not_found, _Res}}) ->
"Error reverting docs : Package or Package Version not found";
format_error(Reason) ->
rebar3_hex_error:format_error(Reason).

5 changes: 2 additions & 3 deletions src/rebar3_hex_file.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
-module(rebar3_hex_file).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whitespace only changes.


-export([
expand_paths/2,
update_app_src/2
-export([ expand_paths/2
, update_app_src/2
]).

expand_paths(Paths, Dir) ->
Expand Down
Loading