-
-
Notifications
You must be signed in to change notification settings - Fork 268
cross repository dependency management #159
Changes from all commits
323a56b
6b0b5b2
1107a57
4eab150
06c4af2
52804e6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
# List the OCA project dependencies, one per line | ||
# Add a repository url and branch if you need a forked version | ||
# | ||
# Examples | ||
# ======== | ||
# | ||
# To depend on the standard version of sale-workflow, use: | ||
# sale-workflow | ||
# | ||
# To explicitely give the URL of a fork, and still use the version specified in | ||
# .travis.yml, use: | ||
# sale-workflow https://github.com/OCA/sale-workflow | ||
# | ||
# To provide both the URL and a branch, use: | ||
# sale-workflow https://github.com/OCA/sale-workflow branchname | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't we want to also have hash support ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think so: I have not seen any There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comments as in doc. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
# List the OCA project dependencies, one per line | ||
# Add a repository url and branch if you need a forked version | ||
|
||
partner-contact https://github.com/OCA/partner-contact 8.0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be also There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a test, not an example. The example is in the sample_files/oca_dependencies.txt |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,100 @@ | ||
#!/usr/bin/python | ||
"""Usage: clone_oca_dependencies [<checkout_dir> <build_dir>] | ||
|
||
Arguments: | ||
|
||
checkout_dir: the directory in which the dependency repositories will be cloned | ||
build_dir: the directory in which the tested repositories have been cloned | ||
|
||
If no arguments are provided, default to the layout used in the OCA travis | ||
configuration. | ||
|
||
The program will process the file oca_dependencies.txt at the root of the | ||
tested repository, and clone the dependency repositories in checkout_dir, | ||
before recursively processing the oca_dependencies.txt files of the | ||
dependencies. | ||
|
||
The expected format for oca_dependencies.txt: | ||
|
||
* comment lines start with # and are ignored | ||
* a dependency line contains: | ||
- the name of the OCA project | ||
- (optional) the URL to the git repository (defaulting to the OCA repository) | ||
- (optional) the name of the branch to use (defaulting to ${VERSION}) | ||
""" | ||
from __future__ import print_function | ||
import sys | ||
import os | ||
import os.path as osp | ||
import subprocess | ||
import logging | ||
|
||
|
||
_logger = logging.getLogger() | ||
|
||
|
||
def parse_depfile(depfile): | ||
deps = [] | ||
for line in depfile: | ||
line = line.strip() | ||
if not line or line.startswith('#'): | ||
continue | ||
parts = line.split() | ||
repo = parts[0] | ||
if len(parts) > 2: | ||
branch = parts[2] | ||
else: | ||
branch = os.environ.get('VERSION', '8.0') | ||
if len(parts) > 1: | ||
url = parts[1] | ||
else: | ||
url = 'https://github.com/OCA/%s' % repo | ||
deps.append((repo, url, branch)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a matter of taste but I prefer named tuple for this kind of usage |
||
return deps | ||
|
||
|
||
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] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can use |
||
_logger.info('Calling %s', ' '.join(command)) | ||
subprocess.check_call(command) | ||
return checkout_dir | ||
|
||
|
||
def run(home, build_dir): | ||
dependencies = [] | ||
processed = set() | ||
for repo in os.listdir(build_dir): | ||
_logger.info('examining %s', repo) | ||
processed.add(repo) | ||
depfilename = osp.join(build_dir, repo, 'oca_dependencies.txt') | ||
dependencies.append(depfilename) | ||
for depfilename in dependencies: | ||
try: | ||
with open(depfilename) as depfile: | ||
deps = parse_depfile(depfile) | ||
except IOError: | ||
deps = [] | ||
for depname, url, branch in deps: | ||
_logger.info('* processing %s', depname) | ||
if depname in processed: | ||
continue | ||
processed.add(depname) | ||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like to modify a list when we are iterating through it with a
(not exactly the same result as Maybe just appending to a list in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Appending to a list in a There are checks in the code to avoid looping, which are not convenient to write if I There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok 🙌 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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". |
||
|
||
|
||
if __name__ == '__main__': | ||
if len(sys.argv) == 1: | ||
home = os.environ['HOME'] | ||
build_dir = osp.join(home, 'build/OCA') | ||
elif len(sys.argv) == 2 or len(sys.argv) > 3: | ||
print(__doc__) | ||
sys.exit(1) | ||
else: | ||
home = sys.argv[1] | ||
build_dir = sys.argv[2] | ||
run(home, build_dir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.