Fix #56 (always-on recursion) #192

Closed
wants to merge 4 commits into
from

Projects

None yet

6 participants

@tuncer
Contributor
tuncer commented Dec 8, 2013

Always-on recursive application of all rebar commands causes too many issues. Recursive application is required for:

  1. dealing with dependencies: get-deps, update-deps, and compile of deps right after get-deps or update-deps
  2. projects with a riak-like apps/ project structure and dev process

The vast majority of projects are not structured like riak. Therefore, moving forward it's best to (by default) restrict recursive behavior to dealing with deps. This branch does that and also adds command line and rebar.config options for controlling or configuring recursion. Also, we introduce two meta commands: prepare-deps (equivalent to rebar -r get-deps compile) and refresh-deps (equivalent to rebar -r update-deps compile). riak-like projects can extend the list of recursive commands (to include 'eunit' and 'compile') by adding {recursive_cmds, [eunit, compile]} to rebar.config.

Fixes #56.

@tuncer tuncer referenced this pull request Dec 8, 2013
Closed

Fix recursion #56

@si14
si14 commented Dec 14, 2013

Wow, the long-awaited change :) Thank you!

@archaelus

At Heroku we've been using this patch on our version of rebar and found it to be great for day to day work. Working with templates, eunit and a variety of other things is far easier than it has been in the past.

💯 I would love to see this patch land in mainline.

@tuncer
Contributor
tuncer commented Feb 14, 2014

@dizzyd's dss-fix-skip-deps branch completely removes skip_deps=. Should I make the same change here? What are the arguments for having both skip_apps= and and skip_deps=?

@jaredmorrow
Contributor

For now I would vote to leave skip_deps, with maybe a deprecation message so it didn't instantly break people's scripts.

@tuncer
Contributor
tuncer commented Feb 14, 2014

Deprecated skip_deps= in favor of skip_apps=.

@tuncer
Contributor
tuncer commented Feb 14, 2014

Added a fix for #226.

@tuncer
Contributor
tuncer commented Mar 4, 2014

ping

@archaelus

Updated version is still good for me - would be great to merge into mainline.

@Vagabond
Contributor
Vagabond commented Mar 5, 2014

What meaning is there in changing skip_deps to skip_apps? It doesn't really convey any additional or clearer meaning, and it is yet another deprecation to deal with.

@jaredmorrow
Contributor

Yes, please remove the skip_apps change and put that in a separate PR. Mixing multiple topics and ideas into a single PR is really difficult to deal with.

@Vagabond
Contributor
Vagabond commented Mar 5, 2014

Compile not being recusive is going to break too much stuff. I just merged the speedup patch, so redundant compiles will be about twice as fast as they were. If we leave compile recursive and back-out the skip_apps change (which I guess you can make different PR for, if you really want) this can merge.

@tuncer
Contributor
tuncer commented Mar 5, 2014

The deprecation message was added as a result of @jaredmorrow's suggestion (see #192 (comment)). I can extract it, if that's preferred.

@tuncer
Contributor
tuncer commented Mar 5, 2014

Compile not being recusive is going to break too much stuff.

For projects that need compile to be always recursive, there's {recursive_cmds, []}. Also, for rebar get-deps compile and rebar update-deps compile there's rebar prepare-deps and rebar refresh-deps. Usually you only need to compile deps once.

We could also get {recursive_cmds,[]} from ~/.rebar/config.

If we really make compile auto-recursive, then there will be a need to override that via rebar.config or the command line for everyone else.

@Vagabond
Contributor
Vagabond commented Mar 5, 2014

So let people use rebar compile skip_deps=true like they did before. If we change compile we break all of the riak repos, all the documentation on rebar, etc. Jared and I are happy with making everything else not be recusrive by default, but compile is too big of a change to swallow.

If we can get the rest of this in, we can revisit the behaviour of compile later, if necessary.

@jaredmorrow
Contributor

My point was not to rename skip_deps to something else. My point was to not remove skip_deps in the first place, and the only way I could reason with that is a deprecation message to skip_deps. Your comment was confusing, making it sound like skip_deps was now renamed to skip_apps. Part of this confusion is on me not even knowing that skip_apps already existed since it nor skip_deps are documented anywhere on the command line.

@jaredmorrow
Contributor

I'm also of the opinion compile should be left as recursive for now since we have the speed up patch merged now, compile will already be way faster. Most books, blogs, etc. that mention rebar, mention ./rebar get-deps compile doing the right thing, breaking that is a lot bigger change IMHO than what people are making it out to be. I'm okay with the rest of this though.

@tuncer
Contributor
tuncer commented Mar 6, 2014

So let people use rebar compile skip_deps=true like they did before.

skip_deps=true is not the same as disabling recursion.

My point was not to rename skip_deps to something else. My point was to not remove skip_deps in the first place, and the only way I could reason with that is a deprecation message to skip_deps. Your comment was confusing, making it sound like skip_deps was now renamed to skip_apps. Part of this confusion is on me not even knowing that skip_apps already existed since it nor skip_deps are documented anywhere on the command line.

See #244. @dizzyd's proposed fix for the recursion problem completely removes skip_deps=, and here we print a deprecation message as suggested by @jaredmorrow. Anything in need of fixing there?

I'm also of the opinion compile should be left as recursive for now since we have the speed up patch merged now, compile will already be way faster. Most books, blogs, etc. that mention rebar, mention ./rebar get-deps compile doing the right thing, breaking that is a lot bigger change IMHO than what people are making it out to be. I'm okay with the rest of this though.

Speed is not the primary motivation to exclude 'compile' from recursive_cmds, but your concerns are valid. I'll try to come up with a good solution to switch compile's recursiveness at runtime. Once that's finished, we can make 'compile' recursive by default and merge the patch.

With that being said, why not make 'compile' non-recursive and release 3.x? I mean, don't we have to bump the major version anyway due to the other changes? Also, if someone runs rebar get-deps compile, we could print a message suggesting to execute rebar prepare-deps instead.

tuncer added some commits Nov 5, 2012
@tuncer tuncer Fix #56 (always-on recursion)
Always-on recursive application of all rebar commands causes too many
issues. Recursive application is required for:
1. dealing with dependencies: get-deps, update-deps, and compile of deps
   right after get-deps or update-deps
2. projects with a riak-like apps/ project structure and dev process

The vast majority of projects are not structured like riak. Therefore,
moving forward it's best to (by default) restrict recursive behavior to
dealing with deps. This commit does that and also adds command line and
rebar.config options for controlling or configuring recursion. Also, we
introduce two meta commands: prepare-deps (equivalent to rebar -r
get-deps compile) and refresh-deps (equivalent to rebar -r update-deps
compile). riak-like projects can extend the list of recursive commands
(to include 'eunit' and 'compile') by adding
{recursive_cmds, [eunit, compile]} to rebar.config.
b3806c5
@tuncer tuncer Deprecate skip_deps= in favor of skip_apps= b40ae6c
@tuncer tuncer Fix #226
Running 'rebar list-templates' can take quite a long time, when it has
to search the file system. To fix that, make list-templates not recurse
by default. To enable recursion, run 'rebar -r list-templates'.
c47a9bc
@tuncer
Contributor
tuncer commented Mar 10, 2014

Added an RFC commit that detects rebar get-deps compile, suggests rebar prepare-deps, and enables recursion as a graceful deprecation measure. If you're okay with that, I'll squash it into the original commit.

@jaredmorrow
Contributor

I don't like you mentioning 'compile' being only temporarily recursive. I think it should always be recursive. I thought I made that clear in previous comments.

@ferd
Contributor
ferd commented Mar 10, 2014

There's little reason for it to be compiled once I've already built my deps once, sadly. I agree that having it recursive only when it follows some kind of inherently recursive call would make sense?

Otherwise it's kind of extremely frustrating when I change a -1 to a +1 in a project of mine and the entire deps tree gets scanned/recompiled for no reason, but yeah.

@jaredmorrow
Contributor

Merged in 47c089a without the non-recursive compile bits.

@tuncer tuncer deleted the tuncer:auto-recursion branch Mar 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment