Remove erlware_commons from the rebar3 codebase#3022
Conversation
* internalize functions of ec_sermver.erl * move ec_semver_parser inside rebar * remove leftover call to erlware_commons * add SPDX tags in erlware files
* remove ec_file:{is_file, is_dir}/1
* remove ec_file:{mkdir_path,mkdir_p}
* remove ec_file:remove/{2,3}
* add missing files for is_dir refactor
* remove ec_file:{write, write_term}/2
* move ec_file:real_dir_path to rebar_file_utils
* move ec_file:insecure_mkdtemp/0 to rebar_file_utils
* remove ec_file:copy/{2,3}
* remove ec_file:is_symlink/1
* add SPDX headers
* remove ec_lists:search/2 from rebar_packages * add test case for check_all_repo correctness * handle_missing_no_exception refactor * remove ec_lists:search/2 * replace ec_lists:find/2 by lists:search/2 + refactoring of callers * add SPDX headers
* refactoring the logs to remove ec_cmd_logs
* remove ec_cnv * restore r3_safe_erl_term * add SPDX headers
ferd
left a comment
ferd
left a comment
There was a problem hiding this comment.
A few small notes.
Main blocker here is that this change edits the provider vendored lib while the original lib we may pull in on upgrades will re-introduce the erlware commons dependency. There's also an app utils type signature change that could be avoided to prevent potential plugin breakage.
| false -> "/tmp"; | ||
| Val -> Val | ||
| end | ||
| end. |
There was a problem hiding this comment.
hm, makes me realize we also have system_tmpdir() in this file. Not sure if we need to eventually remove it, but not a blocker here.
| case check_all_repos(Fun, RepoConfigs) of | ||
| not_found -> | ||
| ec_lists:search(fun(Config=#{name := R}) -> | ||
| check_all_repos(fun(Config) -> |
There was a problem hiding this comment.
you've extracted the name from the Config at line 336, so this variable should no longer be Config but something like RepoName. Either we should keep passing the whole Config (to give more available information to check_all_repos) or rename the variable (to be clear as to what is passed).
| %% @doc finds the proper app info record for a given app name at a given version | ||
| %% in a list of such records. | ||
| -spec find(binary(), binary(), [rebar_app_info:t()]) -> {ok, rebar_app_info:t()} | error. | ||
| -spec find(binary(), binary(), [rebar_app_info:t()]) -> {value, rebar_app_info:t()} | false. |
There was a problem hiding this comment.
This signature change may create more work for plugins that could rely on it down the road. It's up to you regarding future rebar to decide whether it's worth changing it or doing a case to wrap the lists:search call to keep the signature stable. This would make it easier to the merge this into main.
| rebar_utils:filtermap(fun({Plugin, PluginName}) -> | ||
| case rebar_app_utils:find(PluginName, AllPlugins) of | ||
| {ok, _} -> | ||
| {value, _} -> |
There was a problem hiding this comment.
this is one of the places where the change in API (due to an implementation detail) forces more changes downstream.
| %% SPDX-FileCopyrightText: Copyright 2026 Dipl. Phys. Peer Stritzinger GmbH | ||
| %% | ||
| %% %CopyrightEnd% | ||
|
|
There was a problem hiding this comment.
is this a stray change? this is a vendored file that arguably isn't getting touched?
| {<<"optional">>,false}, | ||
| {<<"requirement">>,<<"1.4.0">>}]}, | ||
| {<<"getopt">>, | ||
| [{<<"getopt">>, |
There was a problem hiding this comment.
This is a third-party dependency: https://github.com/tsloughter/providers (see https://github.com/tsloughter/providers/blob/master/rebar.config#L3) -- where the requirement on erlware_commons still exists and would be part of the vendoring relationship, and erlware_commons will still be pulled on fresh revendoring runs.
https://github.com/tsloughter/providers/blob/1cc613c5dc30bd4bfb630c5b261d9e0a952dc2e9/src/providers.erl#L129 is the single call there, which should be easy to excise.
| {erl_opts, [{platform_define, "R14", no_callback_support} | ||
| ,debug_info]}. | ||
| {deps, [{getopt, "1.0.1"}, {erlware_commons, "1.4.0"}]}. | ||
| {deps, [{getopt, "1.0.1"}]}. |
There was a problem hiding this comment.
This code exists at https://github.com/tsloughter/providers, changing it here does not really remove the dependency.
| {pkg_hash_ext,[ | ||
| {<<"cf">>, <<"315E8D447D3A4B02BCDBFA397AD03BBB988A6E0AA6F44D3ADD0F4E3C3BF97672">>}, | ||
| {<<"erlware_commons">>, <<"185ECF5CF43BAB3A013DDB3614CE7BBA7F6C7A827904E64E57DA54FCDFDCE2E6">>}, | ||
| {<<"getopt">>, <<"53E1AB83B9CEB65C9672D3E7A35B8092E9BDC9B3EE80721471A161C10C59959C">>}]} |
There was a problem hiding this comment.
these will get recreated on a fresh vendoring run
| %% @private from a list of app names, fetch the proper app info records | ||
| %% for them. | ||
| -spec names_to_apps([atom()], [rebar_app_info:t()]) -> [rebar_app_info:t()]. | ||
| -spec names_to_apps([binary()], [rebar_app_info:t()]) -> [rebar_app_info:t()]. |
There was a problem hiding this comment.
I'm unsure where this type signature change is coming from. Was it always wrong?
2 participants
As part of our work on the rebar4 kickstarter at Peer Stritzinger GmbH, we are removing externally vendored code from the rebar3 codebase.
erlware_commonsis currently causing compatibility and CI issues with OTP 29. This PR removeserlware_commonsfrom the rebar3 codebase entirely.The work is split into two phases:
erlware_commonsmodules.erlware_commonscode itself.The refactoring was originally developed in smaller steps in our fork, then consolidated here into an upstream-ready branch.