-
Notifications
You must be signed in to change notification settings - Fork 518
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
Initial version of the Manifest plugin (experimental) #2859
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The biggest issue here is that this PR currently disables logging for rebar3 wholesale. This needs to be changed.
The rest of my comments are about improving the robustness for some of the weird cases rebar3 encounters and supports, and minor formatting issues.
|
||
%% By default, the provider outputs the manifest to stdout, so disable logs | ||
%% not to interfere. | ||
ok = rebar_log:init(api, 0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The init/1 function is invoked at Rebar3 start time for all providers to know which commands are available and what their dependencies are. This means that this disables logs for rebar3 universally.
This isn't a good way to go, and if you need logs from stdout, I don't think we can prevent other providers from outputting anything, particularly if you're asking for install_deps
to run already.
The only path forward I see if you want stdout content is to pass in an argument that is going to be an optional delimiter, like --delimiter="~~~content~~~"
which would then give you something like:
~~~content~~~
this is the parseable output
~~~content~~~
Which frankly isn't elegant, but probably workable and without interfering with other unrelated tasks.
ok = rebar_log:init(api, 0), | ||
|
||
State1 = rebar_state:add_provider( | ||
State, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for formatting rules, Rebar3 has historically used 4 spaces indents everywhere in providers. As per the nesting, it tends to look more like https://github.com/erlang/rebar3/blob/main/apps/rebar/src/rebar_prv_repos.erl -- this is not a blocker, but generally preferable.
otp_lib_dir := string(), | ||
source_root := string() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otp_lib_dir := string(), | |
source_root := string() | |
otp_lib_dir := file:filename(), | |
source_root := file:filename() |
|
||
-spec adapt_context(rebar_app_info:t()) -> app_context(). | ||
adapt_context(App) -> | ||
Context0 = rebar_compiler_erl:context(App), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some applications may not compile with that compiler; There's a set of dynamic calls the compiler uses to know if rebar3's own internal compiler handles these:
rebar3/apps/rebar/src/rebar_prv_compile.erl
Lines 330 to 331 in 8207d82
Type = rebar_app_info:project_type(AppInfo), | |
Type =:= rebar3 orelse Type =:= undefined |
Both rebar3
and undefined
types are treated as rebar3 projects. People can configure custom compilers for their apps (such as mix
) and we sort of hand this off to a plugin to handle. So if you want to be safe, you'll want to filter these out via the list comprehensions in lines 110 and 111.
-spec output_manifest(binary(), string() | undefined) -> ok | {error, term()}. | ||
output_manifest(Manifest, undefined) -> | ||
rebar_log:log(info, "Writing manifest to stdout:~n", []), | ||
io:fwrite("~s~n", [Manifest]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without forcing unicode there (with ~ts
) you may get broken results when people have unicode in their file paths in the Erlang format. Then I figure you might also have the problem that the eetf one has invalid unicode sequences. It's possible you need to keep the type with the data to output it safely?
["manifest", "--to", FilePath], | ||
{ok, []}), | ||
{ok, [Manifest]} = file:consult(FilePath), | ||
?assertMatch(#{deps := [], apps := [#{name := AppName}]}, Manifest). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the tests are probably going to be okay, but there's a higher chance we accidentally break compatibility down the road if we aren't stricter on assertions. I'm assuming that your projects will be the main users of this so I'll defer to your judgment on this, we can always add more tests later.
rebar_test_utils:create_app(AppDir, Name, Vsn, [kernel, stdlib]), | ||
%% Add the data to the test config | ||
[{name, unicode:characters_to_binary(Name)} | Config]. | ||
|
||
end_per_testcase(_, Config) -> | ||
Config. | ||
|
||
basic_check(Config) -> | ||
rebar_test_utils:run_and_check(Config, [], | ||
["manifest"], | ||
{ok, []}). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inconsistent spacing here (though I imagine the init_
and end_
cases may have been copy/pasted (which isn't a problem).
Thanks @ferd for the quick and accurate review! I will try to address all the feedback soon.
Regarding documentation about the provider (e.g. in the README or the website), maybe we could leave it undocumented until ELP / Erlang LS integrate with it and the output format is refined. |
Yeah I'm good leaving it undocumented. Alternatively, we have an 'experimental' namespace that lets us change the command as we see fit and announce it and get users but without any guarantees. The issue is of course that once it's stable, if you rely on it already, you need to fallback from default to experimental to unsupported. |
@ferd Sorry for the delay in the follow-up. I think I addressed all the feedback.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems alright! What was the Unicode question?
Also have you given thought to the experimental status of the provider, or do you feel whatever format comes out of this can be relied on to be stable down the road?
|
Oh yeah. The Erlang format is regular text and you'd want that to be unicode-aware, ideally, to keep showing paths as text rather than lists of integers or bytes. The
The Whenever we feel the format is solid and adequate we can promote the task outside the |
@ferd If I mark the plugin as experimental (and update the tests accordingly), I get a failure due to a dependency not found:
It looks like rebar3 assumes that, if a provider is |
oh yeah that's because the plugin dependencies look in your namespace by default. Specify the dep as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, this seems to be in a workable format for an experimental feature. When tests pass I think we'd be good to merge. If dialyzer complains I'll see if I can fast-track a change for the type specs.
@ferd No Dialyzer failure. It looks like the spec fix can be done as a follow-up. |
The
manifest
provider allows users to extract information about arebar3
project, so that the information can be used by external tools such as language servers to learn about the structure of a project. The format and content of the produced manifest is totally up for debate. This is just to start the discussion. Ideally, the manifest should not be tight torebar3
as a build tool. This would enable other build tools to produce an equivalent file.The provider is inspired by the build_info one which is part of the
eqWalizer
type checker for Erlang, but uses built-in functionality to fetch dependencies.Having a built-in provider would remove friction for language server users and implementors. ELP users are currently required to install additional plugins to be able to use the language server.
Erlang LS implementors are attempting efforts such as this one, to approximate the structure of a
rebar3
project. A built-in manifest provider would simplify all of this.Format
Ideally, one could have used JSON as the standard format for the manifest (the Rust Analyzer language server uses a rust-project.json format for a similar concept. Unfortunately, there's currently no JSON library provided out-of-the-box by Erlang/OTP or rebar3, so I'm defaulting to Erlang terms. I am also adding the ability to use
EETF
(Erlang External Term Format) as an alternative format, since this is the format currently used by the ELP language server via thebuild_info
plugin.Output
By default the provider outputs the manifest to stdout. For this reason, logging is disabled by the provider (an alternative would have been to ask users to use the
QUIET=1
option or to filter out extraneous log messages). A dedicated option provides the ability to dump the manifest to file. This is mostly done to simplify testing and to avoid playing with group leaders or similar approaches. Again, this is all open for discussion.Example
Running the command:
For the
Erlang LS project
would produce something like this.