compiler respects 'keep_going' flag #354

vladdu · 2014-09-02T11:11:01Z

I am creating this PR as a base for further discussion.

I found that this change works fine for me, but I don't know if the fact that the worker pid is not removed has any bad effects.

ghost · 2014-09-02T19:06:15Z

In addition, the commit message should be rewritten, so compiler respects 'keep_going' flag should be base_compiler: respect 'keep_going' flag, but a much more useful commit message would be:

Respect -k on a per-file level during compile

To more closely mimic `make -k`, change base_compiler to print an
error and continue building when -k is enabled and a file does
not compile.

Also, we should add tests (extend inttest/erlc or add a new one).

This is a companion patch to go with #322.

vladdu · 2014-09-02T19:18:37Z

Yes, there will be tests and better messages. I wanted just to get a second opinion first.

vladdu · 2014-09-03T16:24:11Z

Note to self: there are more commands that don't respect 'keep-going', for example xref.

ghost · 2014-11-28T15:46:09Z

#322 just got merged and we settled on not turning errors into warnings. It would be great to include this in rebar 2.6, so how soon do you think you can finish the branch?

vladdu · 2014-11-28T18:19:14Z

I will get to it in the next few days.

The only question mark is how to keep track of the worker pid... I'm not sure how to find out which one failed.

Is it ok to move "maybe_report(Error)"? Then the compiled/failed message serves as a marker for when a file has been processed.

ghost · 2014-11-28T20:04:58Z

I will get to it in the next few days.

Thanks, sounds good.

The only question mark is how to keep track of the worker pid... I'm not sure how to find out which one failed.

Is it ok to move "maybe_report(Error)"? Then the compiled/failed message serves as a marker for when a file has been processed.

It should be possible. Let's see what we can figure out.

vladdu · 2014-12-02T08:42:02Z

Updated the commits, added tests.

ghost · 2014-12-03T16:15:49Z

src/rebar_base_compiler.erl

+                true ->
+                    ?WARN("Continuing after build error\n", []),
+                    compile_queue(Config, Pids, Targets)
+                end;


Isn't the indentation wrong here?

ghost · 2014-12-03T16:17:40Z

Any reason to add a test in test/ and not inttest/ (maybe extend inttest/erlc)?

ghost · 2014-12-03T16:19:33Z

@vladdu, should we try to adjust all commands with this pr?

vladdu · 2014-12-03T17:17:41Z

I don't remember why I didn't write a inttest test, Probably because eunit is a bit more familiar. Do you think it would be better to change it?

I think I checked all commands, but I may be remembering wrong. Will check again.

ghost · 2014-12-03T17:23:16Z

I don't remember why I didn't write a inttest test, Probably because eunit is a bit more familiar. Do you think it would be better to change it?

We already have erlc tests in inttest/, so I think it's better to have the new tests there as well.

ferd · 2014-12-04T15:29:23Z

test/rebar_compiler_tests.erl

@@ -0,0 +1,195 @@
+%% -*- erlang-indent-level: 4;indent-tabs-mode: nil -*-
+%% ex: ts=4 sw=4 et


It looks like this entire file is indented with tabs? I think that's against what we usually do, but I can re-edit it when merging if you want.

Weird, probably I used a different editor for that one. I will fix that.

@Tuncer suggested that it would be better to reimplement this as a retest suite, so it will get rewritten anyway.

ferd · 2014-12-04T15:32:14Z

This seems good to me outside of the aesthetic thing.

One remark I have too is that in the last -k support branch we enforced printing errors when detecting something wrong, while here we have warnings. However, the current warnings are added specifically when skipping, so I figure this is okay. Does everyone agree with me on this?

ghost · 2014-12-04T21:10:42Z

@ferd the warnings printed in rebar_base_compiler are fine because an error is printed first. Maybe xref should print something descriptive, though.

ghost · 2015-04-02T11:34:59Z

ping?

ferd · 2015-04-02T13:16:17Z

I'd like this to be rebased to merge it. If we don't hear back, then I'll do it myself, but these are busy days and if I can avoid rebasing this myself, I'd like to.

vladdu · 2015-04-02T13:28:23Z

I'll rebase it.

Is it okay to keep the test as they are, or should they be rewritten as retest tests?

ghost · 2015-04-02T13:30:44Z

It'd be nice to have it as a retest test, but it's fine as is once you fix the formatting issues pointed out by @ferd.

ferd · 2015-04-02T13:31:24Z

I'm guessing the retest suite would provide a better integration, but if the tests run along with everything else and represent actual use cases, I personally do not mind much on this. @Tuncer ?

ghost · 2015-04-02T13:33:39Z

See earlier #354 (comment). It'd be nice as retest tests, but it's fine as is.

vladdu · 2015-04-02T15:04:53Z

I hope the formatting is okay now.

ferd · 2015-04-02T16:30:09Z

It looks good to me. @Tuncer any last comment before I merge this?

ghost · 2015-04-02T22:18:15Z

You can merge it.

compiler respects 'keep_going' flag

ghost mentioned this pull request Sep 2, 2014

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

Merged

ghost reviewed Dec 3, 2014
View reviewed changes

ferd reviewed Dec 4, 2014
View reviewed changes

vladdu added 2 commits April 2, 2015 16:53

compiler and xref respect 'keep_going' flag

ae79184

add tests for 'compiler keep_going'

797d2b6

ferd added a commit that referenced this pull request Apr 2, 2015

Merge pull request #354 from vladdu/compiler_keep_going

c3b09ba

compiler respects 'keep_going' flag

ferd merged commit c3b09ba into rebar:master Apr 2, 2015

vladdu deleted the compiler_keep_going branch April 3, 2015 07:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compiler respects 'keep_going' flag #354

compiler respects 'keep_going' flag #354

vladdu commented Sep 2, 2014

ghost commented Sep 2, 2014

vladdu commented Sep 2, 2014

vladdu commented Sep 3, 2014

ghost commented Nov 28, 2014

vladdu commented Nov 28, 2014

ghost commented Nov 28, 2014

vladdu commented Dec 2, 2014

ghost Dec 3, 2014

ghost commented Dec 3, 2014

ghost commented Dec 3, 2014

vladdu commented Dec 3, 2014

ghost commented Dec 3, 2014

ferd Dec 4, 2014

vladdu Dec 4, 2014

ferd commented Dec 4, 2014

ghost commented Dec 4, 2014

ghost commented Apr 2, 2015

ferd commented Apr 2, 2015

vladdu commented Apr 2, 2015

ghost commented Apr 2, 2015

ferd commented Apr 2, 2015

ghost commented Apr 2, 2015

vladdu commented Apr 2, 2015

ferd commented Apr 2, 2015

ghost commented Apr 2, 2015

		@@ -0,0 +1,195 @@
		%% -- erlang-indent-level: 4;indent-tabs-mode: nil --
		%% ex: ts=4 sw=4 et

compiler respects 'keep_going' flag #354

compiler respects 'keep_going' flag #354

Conversation

vladdu commented Sep 2, 2014

ghost commented Sep 2, 2014

vladdu commented Sep 2, 2014

vladdu commented Sep 3, 2014

ghost commented Nov 28, 2014

vladdu commented Nov 28, 2014

ghost commented Nov 28, 2014

vladdu commented Dec 2, 2014

ghost Dec 3, 2014

Choose a reason for hiding this comment

ghost commented Dec 3, 2014

ghost commented Dec 3, 2014

vladdu commented Dec 3, 2014

ghost commented Dec 3, 2014

ferd Dec 4, 2014

Choose a reason for hiding this comment

vladdu Dec 4, 2014

Choose a reason for hiding this comment

ferd commented Dec 4, 2014

ghost commented Dec 4, 2014

ghost commented Apr 2, 2015

ferd commented Apr 2, 2015

vladdu commented Apr 2, 2015

ghost commented Apr 2, 2015

ferd commented Apr 2, 2015

ghost commented Apr 2, 2015

vladdu commented Apr 2, 2015

ferd commented Apr 2, 2015

ghost commented Apr 2, 2015