-
Notifications
You must be signed in to change notification settings - Fork 303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[IMP] Add abstract dependencies installation #33
Conversation
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.
I love the feature, just some comments to make it fit the project perfectly 😉
BTW, this would need an update in the scaffolding too, but I can do it myself when merging.
build.d/80-dependencies
Outdated
DIR_CUSTOM = '/opt/odoo/custom/dependencies' | ||
|
||
REQ_TYPES = { | ||
'apt': 'apt-get install -y', |
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.
Add --no-install-recommends
build.d/80-dependencies
Outdated
|
||
REQ_TYPES = { | ||
'apt': 'apt-get install -y', | ||
'pip': 'pip install', |
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.
Add --no-cache-dir
.
build.d/80-dependencies
Outdated
'apt': 'apt-get install -y', | ||
'pip': 'pip install', | ||
'npm': 'npm install -g', | ||
'gem': 'gem install', |
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.
Add --no-rdoc --no-ri --no-update-sources
README.md
Outdated
apt_dependencies.txt | ||
gem_dependencies.txt | ||
npm_dependencies.txt | ||
pip_dependencies.txt |
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.
Since the folder is already called dependencies
, do not be redundant and remove the _dependencies
suffix 😊
@@ -200,6 +205,8 @@ Important notes: | |||
A normal [pip `requirements.txt`][] file, to install dependencies for your | |||
addons when building the subimage. | |||
|
|||
### |
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.
I guess you wanted to document this feature, so please do 😉
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.
This one is still empty.
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.
Ooops - I never actually did this
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.
I did it for you, don't worry 😆
build.d/80-dependencies
Outdated
|
||
DIR_CUSTOM = '/opt/odoo/custom/dependencies' | ||
|
||
REQ_TYPES = { |
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.
Call it INSTALL
instead.
build.d/80-dependencies
Outdated
def do_install(command, requirements_file): | ||
with open(requirements_file, 'r') as fh: | ||
requirements = fh.read().strip() | ||
command += ' %s' % requirements |
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.
Yikes, this loop is interesting, but I feel each install method has its things.
For instance,
apt
needs anapt-get update
before installing the build deps, and needsapt-get -y autoremove && rm -Rf /var/lib/apt/lists/*
after installing run deps.pip
can better use its--requirements
flag instead of this read-and-append hack. (maybe not required, see next point).- the rest of methods would benefit of removing lines from its requirements file that start by
#
, so we allow comments there (for pip this would mean the same as the point above).
rm /tmp/*
would be great too at the end of everything.
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.
But I wanted to be lazy! New code will be better 🚀
build.d/80-dependencies
Outdated
with open(requirements_file, 'r') as fh: | ||
requirements = fh.read().strip() | ||
command += ' %s' % requirements | ||
proc = subprocess.Popen(command.split(' ')) |
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.
if you call split without arguments, in case there are multiple spaces it will split correctly, and strip before:
>>> " asdfasdf a sfdasdf ".split(" ")
['', '', '', 'asdfasdf', '', 'a', 'sfdasdf', '', '', '', '']
>>> " asdfasdf a sfdasdf ".split()
['asdfasdf', 'a', 'sfdasdf']
build.d/80-dependencies
Outdated
if name == 'pip': | ||
found_pip = True | ||
|
||
if not found_pip: |
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 can drop this line and stick with the if
below.
build.d/80-dependencies
Outdated
|
||
if not found_pip: | ||
req_file = '/opt/odoo/custom/src/requirements.txt' | ||
if os.path.isfile(req_file): |
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.
Add a deprecation comment here.
c4f3c06
to
c9d889c
Compare
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 have to rebase please.
Design seems great, although there some little clunky pieces such as the apt stuff that should be treated differently depending on if we have apt, apt_build, none or both.
The Install class is OK, but I feel it a little hard to track. I mean that, after all, this code you are adding is not meant to be reused anywhere, so no need to add classes and so on; you can just add some helper methods to the odoobaselib, and then do a simple, hardcoded, sequential script that just makes its checks and installs everything, just like you would do with bash. All will be clearer, and the apt problem will be easier to fix IMHO.
However, as long as it works fine, you are free to use the class-based design if you still feel it's better 😉
@@ -200,6 +205,8 @@ Important notes: | |||
A normal [pip `requirements.txt`][] file, to install dependencies for your | |||
addons when building the subimage. | |||
|
|||
### |
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.
This one is still empty.
lib/installer.py
Outdated
|
||
def _install_apt(self): | ||
self._run_command( | ||
'apt-get update', |
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.
I'd like to avoid running this twice if we have both apt and apt_build. Maybe something like if not self.apt_updated: ...
?
'apt-get -y autoremove', | ||
) | ||
self._run_command( | ||
'rm -Rf /var/lib/apt/lists/*', |
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.
In the same line as above comment, if we have both apt and apt_build, this should go in the last step only, to avoid updating twice.
def _remove_apt(self): | ||
for requirement in self.requirements: | ||
self._run_command( | ||
'apt-get purge -y %s' % requirement, |
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.
Add also an apt-get autoremove -y
after the purge. It will remove all dangling dependencies too.
def _install_requirements(self, command, split=True, shell=False): | ||
if split: | ||
command = command.split() | ||
for requirement in self.requirements: |
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.
I think all of the supported package managers allow passing all dependencies in a single command, can't we drop this loop?
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.
We can, however I found it difficult in my debugging to find which packages were failing. If we try to install everything at once, installation issues have to be handled one at a time. If we separate and install, all the issues can be identified and resolved after the first build or so because we have a complete picture of the situation.
lib/installer.py
Outdated
|
||
def _install_gem(self): | ||
self._install_requirements( | ||
'gem install --no-rdoc --no-ri --no-update-sources', |
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.
Add gem cleanup after this.
|
||
def _install_npm(self): | ||
self._run_command( | ||
'npm install -g', |
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.
Add npm cleanup after this.
shell=shell, | ||
) | ||
|
||
def _run_command(self, command, split=True, shell=False): |
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.
Didn't you add a helper for this already in odoobaselib?
I've already reused the class in #28 & it's used in the removal script too. IMO at the point three scripts are using the same context, a class was a good idea. |
d5f3b19
to
56edd85
Compare
* Add dependencies installation for apt, npm, gem & pip from files in custom/dependencies folder * Support old requirements path
ade9b2f
to
c0c38bd
Compare
c0c38bd
to
dc5c4c8
Compare
Rebase complete & PR comments attended to (except #33 (comment)). Note that I squashed all existing commits into 3ed2d8e in order to make the rebase easier. The commit responding to the most recent review is dc5c4c8 |
I think you guys might like this one. I haven't really tested much yet