Add app name to "OTP release ~ts or later is required" msg #2810
Conversation
|
Tests are passing in Ubuntu and Windows, but failing on macOS ( |
There was a problem hiding this comment.
This is a good change, so thanks for the contribution but it has edge cases. As pointed out in specific comments:
- the old check needs to remain also available
- the check done in the rebar3.erl file needs to remain on the old version that is non-specific because at that point we don't know which app (or if an app at all) has configured this value.
Ideally there would be tests for both call formats as well, since the current proposal migrated all of the old interface to the new proposed one.
| @@ -69,7 +69,7 @@ | |||
| escape_chars/1, | |||
| escape_double_quotes/1, | |||
| escape_double_quotes_weak/1, | |||
| check_min_otp_version/1, | |||
| check_min_otp_version/2, | |||
There was a problem hiding this comment.
we likely want to keep a copy of check_min_otp_version/1 because it's possible plugins started relying on it over the years, and removing the function may break builds in the ecosystem.
There was a problem hiding this comment.
You are right 😄 I'll add it back.
There was a problem hiding this comment.
apps/rebar/src/rebar3.erl
Outdated
| @@ -134,7 +134,9 @@ run_aux(State, RawArgs) -> | |||
| rebar_state:apply_profiles(State, [list_to_atom(Profile)]) | |||
| end, | |||
|
|
|||
| rebar_utils:check_min_otp_version(rebar_state:get(State1, minimum_otp_vsn, undefined)), | |||
| MinimumOTPVsn = rebar_state:get(State1, minimum_otp_vsn, undefined), | |||
| App = rebar_state:current_app(State1), | |||
There was a problem hiding this comment.
Here the app is going to be undefined because we're scanning the config before we have even scanned the applications. Do note that the version check in this file may also come from the global configuration file (in ~/.config/rebar3/rebar.config), in which case any message regarding the application or even the current project might be wrong at this level.
Chances are that since we need to keep the older check's version (see my other comment), we would want to keep using it here when we don't actually know which app configures it or if it's even an app at all.
There was a problem hiding this comment.
From my reading of the code (and I now see I missed a call to rebar_app_info:name/1):
rebar_state:current_app/1returnsundefined | rebar_app_info:t()rebar_app_info:t()'s name isundefined | binary()- I can probably just "try and fetch" name from the potentially defined App and let the newly updated code take care of the rest if it's undefined. Is this a complete solution?
There was a problem hiding this comment.
|
Oh as for the brew thing, yeah it started being really shaky and annoying, so I gradually put the 'brew doctor' recommendation into steps of the MacOS workflow and it seems to fix things :/ I can take a look at it elsewhere if you want to rebase your branch on it when I have it ready. |
I'm gonna add some tests, un-migrating the ones I had migrated, but since the newly introduced function is mostly just a specific case of the existing one, I'm not sure if these add much 🤔 (except, maybe (?) making sure the calls "in the wild" you mention won't break, so that's already something). Edit: 5300c45 |
No description provided.