cross repository dependency management #159

gurneyalex · 2015-01-05T17:04:56Z

use repo/oca_dependencies.txt rather than an explicit list of checkouts in
.travis.yml

closes #105

gurneyalex · 2015-01-05T17:06:40Z

See OCA/vertical-ngo#107 for a test of this implementation.

guewen · 2015-01-06T08:07:38Z

travis/clone_oca_dependencies

+            checkout_dir = git_checkout(home, depname, url, branch)
+            new_dep_filename = osp.join(checkout_dir, 'oca_dependencies.txt')
+            if new_dep_filename not in dependencies:
+                dependencies.append(new_dep_filename)


I don't like to modify a list when we are iterating through it with a for loop.
I prefer to use something like:

while dependencies: depfilename = dependencies.pop() ... if new_dep_filename not in dependencies: dependencies.append(new_dep_filename)

(not exactly the same result as dependencies.pop() will process the dependencies from last to first but I don't think it is an issue here)

Maybe just appending to a list in the for is ok (remove is surely not), but still a bit troubling IMO.

Appending to a list in a for loop is ok. Removing is ok too, as long as you remove after the current iteration element (which is also why appending to a list is perfectly safe: in this case I'm using the list as a queue of elements to be processed). It is even a very nice way to refactor a recursion to an interation 😸

There are checks in the code to avoid looping, which are not convenient to write if I pop() the elements from the queue.

I don't think we should add dependencies recursively for each dependency, because main project can depend on a module of a repository that has dependencies because of another modules, but not the one we need.

Making this, we can increase a lot the build time (specially with those branches that has a lot of commits or branches - this will get worse with each Odoo version).

Each repository maintainer should resolve manually the dependency chain and add it to main project.

I am with @pedrobaeza here. Repos are loose groups of modules, and I am not very keen to work too much on repo dependencies (a concept I'd like to get rid of eventually). We already have the proprietary odoo dependency resolution, and we're adding even another one.

Still, I understand how that makes life a bit easier, and I am not blocking it. Thanks!

I disagree.

This will reduce breakage when other repositories' external dependencies change. And I personnally hate having to manually track these when a computer can perfectly do the job in my stead (and should do so).

@pedrobaeza Built time increase argument is void. The resulting clones are currently exactly the same as the one we are manually configuring, and the checkout time of OCA repositories is negligible compared to creating the base database.

@gurneyalex, no if added dependencies (for new modules) are not used by our repository. For example, spanish localization uses base_location of partner-contact repository. Only that module. But module partner_relations needs web repository because it uses web_m2x_options and web_tree_many2one_clickable, and thus this dependency is added at partner-contact level.

The way you make it, it will download also project web, and the corresponding dependencies for it, and so on recursively, needing to download practically all OCA repos for each Travis test, when I only need for spanish localization to download partner-contact repo.

I also agree with Pedro. I would rather see "nested" dependencies be explicitly declared. I also hope this gives incentive keep dependencies to a minimum and try to stay away from a "monolith".

guewen · 2015-01-06T08:43:15Z

👍

nbessi · 2015-01-06T08:55:32Z

travis/clone_oca_dependencies

+            url = parts[1]
+        else:
+            url = 'https://github.com/OCA/%s' % repo
+        deps.append((repo, url, branch))


Just a matter of taste but I prefer named tuple for this kind of usage

yvaucher · 2015-01-06T09:04:04Z

@gurneyalex in case of LINT check only do we want to clone deps ? Could we have a way of avoiding this unnecessary step ?

gurneyalex · 2015-01-06T09:30:07Z

@yvaucher this is already taken care of in travis_install_nightly (https://github.com/OCA/maintainer-quality-tools/blob/master/travis/travis_install_nightly#L6) and I have not changed this part.

yvaucher · 2015-01-06T09:37:58Z

@gurneyalex oh right thanks

Then 👍

nbessi · 2015-01-06T10:06:04Z

👍

pedrobaeza · 2015-01-06T17:52:06Z

README.md

-    install:
-      - git clone https://github.com/OCA/a_project_x ${HOME}/a_project_x -b ${VERSION}
-      - git clone https://github.com/OCA/a_project_y ${HOME}/a_project_y -b ${VERSION}
+    project_name optional_repository_url optional_branch_name


You should put OCA_project_name and put the other two parameters enclosed by brackets, that it's the common terminology for regular expressions with optional parts.

that's the common way for USAGE strings output when --help is passed as a command line option... In the context of an example configuration I tend to find my version clearer.

Waiting for more feedback on this.

nbessi · 2015-01-28T13:14:12Z

@dreispt @pedrobaeza can we merge this one ?

pedrobaeza · 2015-01-28T13:16:33Z

My concern about dependency resolution hasn't been addressed as far as I can see.

guewen · 2015-02-06T08:12:46Z

travis/clone_oca_dependencies

+def git_checkout(home, reponame, url, branch):
+    checkout_dir = osp.join(home, reponame)
+    if not osp.isdir(checkout_dir):
+        command = ['git', 'clone', '-q', url, '-b', branch, checkout_dir]


I think we can use depth=1 to clone faster.

guewen · 2015-02-06T08:19:40Z

My concern about dependency resolution hasn't been addressed as far as I can see.

Is it only a matter of build time? I'm not sure that it is really a concern...

git clone git@github.com:OCA/sale-workflow.git --depth=1

takes 2 seconds on my computer

pedrobaeza · 2015-02-06T09:08:50Z

--depth=1 solution seems good to me

yvaucher · 2015-02-06T10:31:48Z

Ok I had a look at our repo and their dependencies, and I changed my mind.

I don't think there is a case we would download a monolithic source base.

This for few reasons:

Our dependency tree has a low depth
Topics alwost always implies that if you are pulling a repo you will need all its dependencies.
Many repo still have no dependencies

Thus for the few case we would pull extra repo, it's not worth the time to have to write down everywhere the exact list needed.

We can later improve this automatically searching deps in __openerp__.py certainly be writing down somewhere the hierarchy to avoid too many connexion to github repo.

In 9.0 we won't have this issue anymore if 1 repo = 1 module

StefanRijnhart · 2015-02-06T10:40:36Z

@yvaucher that is a big if, isn't it?

yvaucher · 2015-02-06T10:44:37Z

@StefanRijnhart For 9.0 more I think about it and more I'm convinced it would be the best way to go to manage dependencies.

dreispt · 2015-02-07T22:36:42Z

Saying that a repo depends on another is a misleading way to put it.
It's more accurate to say that some modules in a repos depend on another.

We can have a module in repo A depend on a module in repo B, and another module in repo B depend on a module in repo C.
This makes repo A "depend" on repo B but not on repo C.
In math jargon, it's a non transitive relation. 👽

Repos are organized around common functional areas.
Inter-repo dependencies are to be avoided, or at least, not incentivized.
We should work to keep complexity down having modules as decoupled as possible,
Inter-repo dependencies are still to be expected, but mostly in border areas.

I believe that having a border module in A depends on a border module of B that needs a module in C should be a rare situation.
And since it's not desirable to have too much coupling, I would prefer to have these cases explicitly identified.

So I don't sympathize with the auto discovery of dependencies.
But feel free to go on if I'm the only one with this opinion.

pedrobaeza · 2015-02-08T00:32:58Z

@dreispt, I think similar to you, but this can be minimized if the auto-discovery doesn't penalize performance.

lepistone · 2015-02-11T10:05:46Z

So a few of us, me included, are a bit skeptical of bringing the idea of repo dependencies too far.

Recently, I have been working on modules like stock-dropshipping-dual-invoice that clearly deal with stock, sales and purchases, so it is easy to see what modules it depends on, but the repos it touches (and makes dependent on each other) are much less clear. With modules like that, the idea of independent repositories seems to me difficult, useless and counterproductive.

It is indeed useful to group modules for human reasons (determining the maintainer, listing them clearly), but technically, I'd love to get rid of module dependencies one day (with one-module repos, probably).

TL;DR here what @gurneyalex proposes addresses a problem we have today, and he did the work already, so I am 👍 for merging anyway! Is anyone against?

yvaucher · 2015-02-11T12:11:38Z

needs a rebase

use repo/oca_dependencies.txt rather than an explicit list of checkouts in .travis.yml

silence pip error messages especially the ones caused by the installation of Pylint. silence rm if no file to remove, quiet wget and apt-get

bwrsandman · 2015-03-10T13:21:11Z

👍 I am for as well

It can help greatly for external tools such as travis, but also runbot, documentation, translation and all sorts of other test/utility servers.

dreispt · 2015-03-10T13:49:39Z

@gurneyalex I think you have 4 thumbs up.

max3903 · 2015-03-10T13:57:24Z

👍

cross repository dependency management

gurneyalex added the work in progress label Jan 5, 2015

gurneyalex force-pushed the dependency_management branch 5 times, most recently from d3e7219 to 39d89b9 Compare January 6, 2015 07:43

guewen reviewed Jan 6, 2015
View reviewed changes

gurneyalex force-pushed the dependency_management branch 3 times, most recently from e95a4a9 to 4d3cef2 Compare January 6, 2015 08:19

gurneyalex added needs review and removed work in progress labels Jan 6, 2015

nbessi reviewed Jan 6, 2015
View reviewed changes

yvaucher mentioned this pull request Jan 6, 2015

Update sample files #158

Merged

gurneyalex mentioned this pull request Jan 6, 2015

add a dependencies.txt file at repository level #105

Closed

pedrobaeza reviewed Jan 6, 2015
View reviewed changes

dreispt mentioned this pull request Jan 30, 2015

add oca_dependencies.txt OCA/department#30

Merged

1 task

nbessi mentioned this pull request Feb 4, 2015

add oca_dependencies.txt OCA/connector-interfaces#9

Closed

guewen reviewed Feb 6, 2015
View reviewed changes

gurneyalex added 6 commits February 11, 2015 16:51

cross repository dependency management

323a56b

use repo/oca_dependencies.txt rather than an explicit list of checkouts in .travis.yml

reduce verbosity of travis_install_nightly

6b0b5b2

silence pip error messages especially the ones caused by the installation of Pylint. silence rm if no file to remove, quiet wget and apt-get

add sample oca_dependencies.txt

1107a57

remove example dependency in sample travis cfg

4eab150

update documentation

06c4af2

use oca_dependencies.txt in the tests

52804e6

gurneyalex force-pushed the dependency_management branch from 4d3cef2 to 52804e6 Compare February 11, 2015 15:51

guewen added a commit that referenced this pull request Mar 20, 2015

Merge pull request #159 from gurneyalex/dependency_management

b43cc48

cross repository dependency management

guewen merged commit b43cc48 into OCA:master Mar 20, 2015

guewen removed the needs review label Mar 20, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cross repository dependency management #159

cross repository dependency management #159

gurneyalex commented Jan 5, 2015

gurneyalex commented Jan 5, 2015

guewen Jan 6, 2015

gurneyalex Jan 6, 2015

guewen Jan 6, 2015

pedrobaeza Jan 6, 2015

lepistone Jan 7, 2015

gurneyalex Jan 13, 2015

pedrobaeza Jan 13, 2015

dreispt Jan 14, 2015

guewen commented Jan 6, 2015

nbessi Jan 6, 2015

yvaucher commented Jan 6, 2015

gurneyalex commented Jan 6, 2015

yvaucher commented Jan 6, 2015

nbessi commented Jan 6, 2015

pedrobaeza Jan 6, 2015

gurneyalex Jan 13, 2015

nbessi commented Jan 28, 2015

pedrobaeza commented Jan 28, 2015

guewen Feb 6, 2015

guewen commented Feb 6, 2015

pedrobaeza commented Feb 6, 2015

yvaucher commented Feb 6, 2015

StefanRijnhart commented Feb 6, 2015

yvaucher commented Feb 6, 2015

dreispt commented Feb 7, 2015

pedrobaeza commented Feb 8, 2015

lepistone commented Feb 11, 2015

yvaucher commented Feb 11, 2015

bwrsandman commented Mar 10, 2015

dreispt commented Mar 10, 2015

max3903 commented Mar 10, 2015

cross repository dependency management #159

cross repository dependency management #159

Conversation

gurneyalex commented Jan 5, 2015

gurneyalex commented Jan 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guewen commented Jan 6, 2015

Choose a reason for hiding this comment

yvaucher commented Jan 6, 2015

gurneyalex commented Jan 6, 2015

yvaucher commented Jan 6, 2015

nbessi commented Jan 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbessi commented Jan 28, 2015

pedrobaeza commented Jan 28, 2015

Choose a reason for hiding this comment

guewen commented Feb 6, 2015

pedrobaeza commented Feb 6, 2015

yvaucher commented Feb 6, 2015

StefanRijnhart commented Feb 6, 2015

yvaucher commented Feb 6, 2015

dreispt commented Feb 7, 2015

pedrobaeza commented Feb 8, 2015

lepistone commented Feb 11, 2015

yvaucher commented Feb 11, 2015

bwrsandman commented Mar 10, 2015

dreispt commented Mar 10, 2015

max3903 commented Mar 10, 2015