Run update-deps hooks again #277

Open
wants to merge 5 commits into
from

Projects

None yet

4 participants

@Vagabond

Fix regression caused by making update-deps actually work.

Vagabond added some commits Mar 12, 2014
@Vagabond Vagabond Run pre/post hooks for update-deps
This is a bit of a kludge, but because we are operating outside of
rebar_core when doing update-deps it seems necessary.
4bb4375
@Vagabond Vagabond Actually run the hooks, not just the plugins f911f8f
@Vagabond Vagabond Fix handling of raw deps f1b1dd8
@Vagabond Vagabond Handle sub_dirs in update-deps 780b85c
@Vagabond Vagabond Unused vars
f23af3a
@tuncer tuncer commented on the diff May 9, 2014
src/rebar_core.erl
@@ -28,6 +28,8 @@
-export([process_commands/2, help/2]).
+-export([execute_pre/5, execute_post/5, apply_hooks/4]).
@tuncer
tuncer May 9, 2014

We should probably add a %% for internal use only comment.

@tuncer tuncer commented on the diff May 9, 2014
src/rebar_deps.erl
@@ -47,6 +47,8 @@
source,
is_raw }). %% is_raw = true means non-Erlang/OTP dependency
+-record(subdir, {dir }).
@tuncer
tuncer May 9, 2014

Deliberate space before } in {dir }?

@tuncer

Hi @Vagabond, even though it may fix a regression, I don't think rebar_core functions should be called by anything but rebar.erl. Actually, we may have a conceptual design flaw to fix instead. The initial update-deps kludge added to rebar_core was tolerable, but this one doesn't seem right.

Regardless, if we're going to merge this, you should squash the commits, as the last four look like fixups for the first commit. Also, 780b85c introduces overlong lines, and a regression test would be great.

cc @dizzyd

@Vagabond

Yeah, it is unfortunate, but I was unable to find another way to do it. I do have plans to fix the conceptual flaw, but it is unclear if/when I'll have to time to actually write that code.

@tuncer

It sounds like the chances of somebody fixing it properly will be much lower once we merge the kludge. With that being said, if you provide a regression test, I'll try to investigate a proper fix.

@tsloughter
Rebar member

So this fixes a regression, right? I don't think the 'kludge' is much. I think we should get this in soon...

@tuncer

@tsloughter wrote:

So this fixes a regression, right? I don't think the 'kludge' is much. I think we should get this in soon...

It turns the code inside out, and this appears to be an unfortunate regression caused by #142. Of course we should fix regressions, but this one should be followed up with a proper fix that removes the kludge. Also, it's best to add a NOTE/TODO comment explaining the kludge (in rebar_core and rebar_deps) and what needs to be fixed.

@ferd ferd added the bug label Jun 15, 2014
@ferd ferd referenced this pull request Oct 17, 2014
Open

Slow update-deps #369

@tuncer

So, what do we do about this? As I said earlier, given a clear NOTE/TODO comment, it's okay to merge, assuming this fixes the regression.

@ferd

I frankly have no idea. I don't understand this code base, don't understand the implications, never know what may break or not. I'm fine with merging it and reversing if it breaks more stuff, though.

@tuncer

So, how about merging this to master after the next release, and see if anything breaks?

@ferd

Sure, that sounds like a plan. The current release is still blocked on getting the proper protobuffs configs fixed, though.

@lrascao lrascao referenced this pull request Jan 22, 2016
Open

Stale branches #561

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment