Treat vsn mismatch as warning if -k/--keep-going #322

Merged
merged 1 commit into from Nov 27, 2014

Projects

None yet

4 participants

@tuncer
Contributor
tuncer commented Jul 13, 2014

Fixes #319.

@ferd
Contributor
ferd commented Jul 14, 2014

@archaelus can you confirm this branch works for you?

@archaelus

This all seems to work to me. In my branch I tried:

check_versions(Config) ->
    %% ...
    KeepGoing = rebar_config:get_xconf(Config, keep_going, false),
    %% ...
     case re:run(erlang:system_info(version), ErtsRegex, ReOpts) of
         match ->
             ?DEBUG("Matched required ERTS version: ~s -> ~s\n",
                    [erlang:system_info(version), ErtsRegex]);
        nomatch when KeepGoing ->
            ?WARN("ERTS version ~s does not match required regex ~s\n",
                  [erlang:system_info(version), ErtsRegex]);
        nomatch when not KeepGoing ->

And so on. It shortens the patch a little by not introducing a maybe abort function.

Is there any opinion on which of the force or keep_going flags is more appropriate for this? I went with keep_going as it seemed less likely to force things other than version requirements to be squashed.

@archaelus

I can confirm this branch works and solves my problem.

@tuncer tuncer changed the title from Treat vsn mismatch as warning if -f is passed to Treat vsn mismatch as warning if -k/--keep-going Jul 14, 2014
@tuncer
Contributor
tuncer commented Jul 14, 2014

@archaelus I didn't want to repeat the log message and added a small function.

I agree that -k/--keep-going makes more sense and revised the patch.

@archaelus

👍

@tuncer
Contributor
tuncer commented Jul 14, 2014

Thinking about it, the current behavior of -k/--keep-going is to keep going if a command fails and it's handled in rebar_core's main loop. force affects rebar generate and rebar create. So, while -k seems like the better choice, I'd like to hear more opinions before we potentially change the meaning of -k. @dizzyd @ferd @jaredmorrow?

@ferd
Contributor
ferd commented Jul 15, 2014

That would depend on whether we consider the version check to act like an implicit command (I'm coining a term there) or not. If so, we can decide that -k does mean "keep-going if a command fails or some pre-condition isn't met". If not, we'll need to add a new flag (like -u --unsafe) unless we feel like releasing Rebar 3.0.

@tuncer
Contributor
tuncer commented Jul 15, 2014

Current behavior:

  • rebar -k compile eunit will stop compiling after the first failed file but still run eunit
  • rebar -k compile will abort if any file cannot be compiled

Once the -k patch is merged, users might request/expect rebar -k compile to continue compiling until all files are processed. I don't know how likely it is for users to request that, but as far I can tell it's what make -k does.

@ferd
Contributor
ferd commented Jul 15, 2014

Yeah that does seem to break backwards compatibility right there, unless we consider the current -k behaviour to be buggy and we feel it should keep going no matter what (i.e. work like make's)

@tuncer
Contributor
tuncer commented Jul 15, 2014

What does make -k build test do if any of the files processed in build fails? If -k is actually applied on both levels, then it's fine to do the same in rebar.

@ferd ferd removed the Minor label Jul 16, 2014
@vladdu
Contributor
vladdu commented Aug 31, 2014

I am one of the people that expected -k to keep compiling all files, because I want to have a list of errors/warnings as complete as possible.

@tuncer
Contributor
tuncer commented Sep 2, 2014

After discussing this with @vladdu and consulting the make manpage, this seems like the right behavior. If everybody agrees, and you can verify we didn't misinterpret make -k behavior and docs, let's merge this. @vladdu, do you think we should print an ERROR instead of WARN message before continuing like in #354?

@vladdu
Contributor
vladdu commented Sep 2, 2014

The similar situation at https://github.com/vladdu/rebar/blob/master/src/rebar_core.erl#L113 is handled with a WARN. I don't have a strong opinion, but I think they should be tha same.

@ferd
Contributor
ferd commented Sep 2, 2014

I'm a +1 on this with a WARN only.

@vladdu
Contributor
vladdu commented Sep 2, 2014

The error report would already have printed the error message, no? The warning tells the user that the following errors might not be real, but follow-up errors.

@tuncer
Contributor
tuncer commented Sep 2, 2014

I think for consistency printing an ERROR + "continuing..." message seems reasonable to me to signal it was ignored/overridden but still a real error.

@tuncer
Contributor
tuncer commented Nov 25, 2014

According to make docs, -k doesn't turn errors into warnings, so I'd say ERROR is the right choice. So, let's settle the log level debate and move this plus a finished version of #354 to master.

@ferd ferd merged commit 167e2a1 into rebar:master Nov 27, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@tuncer tuncer deleted the tuncer:fix-319 branch Nov 27, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment