Skip to content

Honor local deps over code:lib_dir #47

Merged
1 commit merged into from Mar 3, 2011

4 participants

@skeptomai

When running 'check-deps' or 'get-deps' , current master branch will be satisfied with a dependency found in the code path rather than checking the local deps first. Where that becomes dangerous is something like the following (from a rebar.config)

{deps, [
{mochiweb, "1.*", {git, "https://github.com/mochi/mochiweb.git", "HEAD"}}
]}.

If mochiweb 1.5 is found in the code path (i.e. copied into the OTP release or other lib path added at load time) but mochiweb doesn't exist in the project's deps directory, the dependency is still satisfied. Imagine mochiweb 1.6 exists in the git repo. Consistent behaviour would dictate we always check the dependency source and attempt to fulfill it locally (in this case, making sure the developer knows to run 'get-deps' and pull from git) to avoid confusion. That's what my patch does.

@joewilliams

I haven't had a chance to look at the patch but I agree that local dependencies should take precedence over external ones.

@hyperthunk

I think that rebar should try and find the closest match to what the user asked for. I'm not sure agree with the example given - if we go by the principle of least surprise, without documentation to the contrary, I would expect the version that I ask rebar for to drive the decision, not the state of a git repository. I suspect many like myself read the third element of the dependency tuple to mean where I get this from if it isn't installed in code:lib_dir already, which doesn't clearly imply that the state of the source repository drives everything.

If the dependency in rebar.config specified mochiweb-1.6 explicitly, then I agree that rebar should not accept a lower version installed in code:lib_dir but if I've asked for mochiweb-1.* in there is a matching version already installed, I don't automatically expect the state of the external repository in which mochiweb is hosted to come into play. I also wonder whether there is an overlap here with the functionality surrounding {vsn, VCS} support in app.src files?

I think this needs further discussion on the list. I'd happily work around it as I tend to be very specific about versions in my dependency declarations (i.e. asking for 1.6 explicitly rather than 1.*) but that's just me. We need to avoid surprising people.

@skeptomai

I agree with the principle of least surprise, but I don't agree that's what is occurring today. When given a rebar.config that specifies a source for dependency D1 and dependency D1 currently installed in code:lib_dir, the current code will say the dependency is met (with check-deps) but will fetch from git with get-deps and build that local version, but then use the version in code:lib_dir. That's not least surprise to me, as I had no idea what was happening until I ran with -v.

When given a source, I interpret that to mean "I've told you this dependency exists in my source repo, please use it to the exclusion of all else" because that gives consistent results across many developers and their disparate configurations. This is less surprise to a group of developers with differing global environments and is also less risky in a "build into deployment" environment.

It's also less surprise in the 1.6 vs 1.6* or 1.* cases, as the behaviour will not differ (i.e. it will fetch and use a local dep) even when the minor version installed elsewhere moves over time; again I contend this gives greater predictability and less surprise.

@hyperthunk

Yeah you're absolutely right about the scenario you describe with D1 and that's messed up - needs to change. What I'm not sold on is when $ERL_LIBS/D-1.6 already exists and I've asked for {D, "1.6", _} I don't expect get-deps to go fetch it again locally. I think I'm used to the maven/rubygems/bundler way, in so much as once I've got a thing locally I don't need to get it again. Maybe the answer to this is to point my {deps, Path} tuple to $ERL_LIBS by default, I don't know.

I see where you're coming from in terms of consistency - forcing the local deps folder to take precedence means that every developer gets the same working environment under the project's root directory after the first build. It can be annoying if you're not connected to the internet and rebar clean blows this away though - suddenly I can't build anything despite having a version of D-1.6 installed into $ERL_LIBS but maybe there isn't a perfect solution to this. Here's my thoughts then:

  1. I agree that honouring local deps makes more sense for a group of devs as they know they get the same environment under the project's root directory
  2. I think the interpretation of what {_,_Source} means in the deps tuple varies, so we need to document this clearly
  3. I would like not to have to be connected to the internet to build my project and I find having rebar clean blow away my deps folder very annoying in this case - this is why I've preferred to install deps to $ERL_LIBS once and rely on that instead of constantly re-downloading everything.

I guess point 3 isn't to do with this patch, so it's something I should raise as a separate issue. The other issue is where I've got nested modules that depend on each other as in this demo. When I want to have to sub folders that contain components and these might depend on each other, I'd like to see how your patch affects the build process. I'm hoping that the commands will still work as expected because the inclusion of pwd in $ERL_LIBS will cause check-deps to note the dependency is satisfied and therefore get-deps won't fetch it. Have I interpreted that correctly?

@skeptomai

Very clear assessment and I agree on the 'clean-deps' thing. (#3) That would really be annoying. Oddly enough, that's almost how I got to this patch. I have something installed under the OTP release and was bitten by that being chosen as the dependency and went searching for why.
Funny you mention bundler, as I was discussing that exact comparison earlier today. I hadn't thought at all about pointing deps at $ERL_LIBS. Perhaps that is a good approach. I defer on that one.

I'll check out that demo and see what my patch does.

@hyperthunk

Please also take note of this pull request. The issue there was that running commands like clean would screw up the state of external dependencies. If we are going to move towards favouring local deps instead, I'd still like to avoid having to run through their entire build process every time I want to interact with my own code. There is skip_deps=true on the command line I suppose, but it is very annoying to have to keep doing that and is the sort of bug-bear that gets people writing wrapper scripts and makefiles - things that I'd like to get away from. In one case I'd created a sub-module and set {skip_deps, "true"} in the rebar.config but even that didn't prevent clean from processing the dependencies every time. It's just really annoying compared to bundler - we should be able to find a better way of doing this stuff.

@skeptomai

tl;dr : I tried my patch on the nested modules demo in two ways, both eventually successful:

1) I altered mod_b's rebar.config, removing the source to see if it would be found locally via ERL_LIBS
e.g.
{deps, [{mod_a, "0.0.1"}]}.
That worked with the following output (using -v)

INFO: is_app_available, looking for App mod_a with Path "/Users/cb/Projects/rebar-nested-modules/lib/mod_a"
WARN: Expected /Users/cb/Projects/rebar-nested-modules/lib/mod_a to be an app dir (containing ebin/*.app), but no .app found.
INFO: is_app_available, looking for App mod_a with Path "/Users/cb/Projects/rebar-nested-modules/mod_a"
INFO: Looking for mod_a-0.0.1 ; found mod_a-0.0.1 at /Users/cb/Projects/rebar-nested-modules/mod_a

Notice it tried to find the dependency in the specified deps dir "lib" and when it didn't, it found it by probing the path. That works.

2) I did 'rebar get-deps', causing it to retrieve the dependency from git. I think there's a minor error in the rebar.config:
{deps, [{mod_a, "0.0.1", {git, "git://github.com/nebularis/rebar-nested-modules-b.git", "master"}}]}.

Notice the dependency is supposed to be for mod_a but is fetching mod_b from git. Once I changed that to "rebar-nested-modules-a.git", it also works, and it finds the dependency in the deps dir "lib":

INFO: is_app_available, looking for App mod_a with Path "/Users/cb/Projects/rebar-nested-modules/lib/mod_a"
INFO: Looking for mod_a-0.0.1 ; found mod_a-0.0.1 at /Users/cb/Projects/rebar-nested-modules/lib/mod_a
DEBUG: Available deps: [{dep,"/Users/cb/Projects/rebar-nested-modules/lib/mod_a",
mod_a,"0.0.1",
{git,"git://github.com/nebularis/rebar-nested-modules-a.git",
"master"}}]

@hyperthunk

Hey, thanks for the investigative work there - that's really useful to know. We should probably document how nested modules work on the rebar wiki - I'll propose some additional content on the mailing list.

I think the patch looks well suited to even complex edge cases like my nested modules demo and am comfortable with the change in behaviour - let's see what the core developers think!

@dizzyd
dizzyd commented Mar 1, 2011

My apologies for not keeping a closer eye on this discussion -- I do wish GH would be a bit more helpful in notifying me when these things are updated. :) skeptomai, I believe that your reasoning about local deps being honored first is sound and something we should clearly document/fix. I'm testing your branch this evening and will merge if it all looks good.

Thanks!

@skeptomai

No worries man! Thanks for looking it over and let me know if there's anything I can do.

@dizzyd
dizzyd commented Mar 1, 2011

OK, here are my thoughts from a quick-n-dirty code review:

  • get_deps_dir/0,1 is confusing/overloaded since it now takes an app argument
  • get_lib_dir/1 is just a wrapper around code:lib_dir...but why?
  • Why break out find_dep/1,2? It looks like it's (roughly) same code as before just spread across multiple functions.
  • ?INFO should really be a ?DEBUG

In terms of general direction, I agree that if a source is provided it should be used to the exclusion of things installed in OTP root. That does yield better, more predictable semantics.

So, if you can address these topics and squash the commit for clarity, that would be great. Thanks for the research and work. We'll make deps generally useful yet... :)

@skeptomai

Thanks! Before I make any edits, let me give the rationale behind a few of the changes to get suggestions to improve them.

  • get_deps_dir/0,1 is confusing/overloaded since it now takes an app argument

All of the uses, except one, in the original code did a
filename:join(get_deps_dir, Dep#app), incurring both an extra
filename:join and less clear in the intent. I upfactored the app
argument and did a single filename:join. The only use of the arity 0
is in delete-deps. I could create a get_app_dep_dir or something, but
we'd probably be back to multiple filename:joins, one in and one out
of the function

  • get_lib_dir/1 is just a wrapper around code:lib_dir...but why?

This was for clarity of intent in the name and symmetry with
get_deps_dir. The original code had this in is_app_available/0 and
was the only use of that arity 0 function.

case code:lib_dir(App) of
    {error, bad_name} ->
        {false, bad_name};
    Path ->
        is_app_available(App, VsnRegex, Path)

Now there's only is_app_available/3. (For comparison, the new use of is_app_available is cleaner)

  • Why break out find_dep/1,2? It looks like it's (roughly) same code as before just spread across multiple functions.

Ah, I wanted to make clear that the real differentiator for pursuing
local deps is the presence of the 'source' argument. The consuming
code above only knows find_dep/1, but its internal usage
differentiates between with source and without.

@dizzyd
dizzyd commented Mar 2, 2011

Long and short of it: thanks for providing the context -- I had to dig up some history to make sense of this stuff again. I appreciate your willingness to defend your decisions...

  • Regarding get_deps_dir, I understand and agree with your refactoring -- it was just a confusing change to me since it changed what the function did without changing the name. That said, I can see where get_deps_dir could have an alternative reading of "get the directory for dep X" versus my reading of "get the directory where deps exist" (and the original intent of that name).

  • get_lib_dir evolved as we worked towards getting better error codes; had to do some serious thinking to remember that. :)

  • I think if we can just add some docs around find_dep/1,2 that might go a long way towards explaining the intent. I personally still find it a confusing read, but if Tuncer can make sense of it am willing to chalk it up to my own history with the code. :)

@skeptomai

I've documented the edited functions, rebased to latest master, and squashed the earlier commits onto that.
It's again on skeptomai:local_deps.

@dizzyd
dizzyd commented Mar 3, 2011

I'm still seeing 3 commits (versus a single squashed) -- is that the intent? Otherwise, I'm fine to merge.

@skeptomai

Nope, I'm just cack-handed and committed incorrectly. Should be correct now on skeptomai:local_deps. Sorry for the confusion.

@dizzyd
dizzyd commented Mar 3, 2011
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.