Issue 29526588: Issue 5559 - include Node.js in ensure_dependencies.py (Closed)
|
Created:
Aug. 24, 2017, 11:04 a.m. by tlucas Modified:
Aug. 28, 2017, 8:55 a.m. CC:
kzar Visibility:
Public. |
DescriptionIssue 5559 - include Node.js in ensure_dependencies.py
Patch Set 1 #
Total comments: 27
Patch Set 2 : Refactoring according to new integration notes #
Total comments: 2
Patch Set 3 : #
Total comments: 6
Patch Set 4 : #MessagesTotal messages: 13
Patch Set 1 Let ensure_dependencies.py install Node.js dependencies.
https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies.py File ensure_dependencies.py (right): https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:258: Do nothing when the SKIP_DEPENDENCY_UPDATES environment variable is set. SKIP_DEPENDENCY_UPDATES should be handled implicitly if this only runs on repository update. https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:267: # production-dependencies declared, we don't need to run npm and can Nit: "production dependencies" https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:279: } I'd rather have the arguments specified in the actual call rather than in a separate dictionary. https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:281: if subprocess.call(['npm', 'outdated'], **subprocess_kwargs) == 0: Given that `npm outdated` hits the network, I'd rather not call it - just call `npm install` directly. Besides, this doesn't work correctly anyway, with `npm outdated` listing outdated development dependencies as well. https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:292: output, error = npm_process.communicate() Better use subprocess.check_output() - this will set up stdout and raise errors automatically. As to stderr output from npm, we should not suppress it IMHO. https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:293: logging.info('Node.js: ' + output) It's npm output, not Node.js. Besides, I doubt that the module tree produced here is helpful in any way, I'd rather suppress the output. https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:392: resolve_npm_dependencies(target, vcs) This will be called every time, not merely when the repository is updated. `npm install` requires network access and might be a heavy operation, so we shouldn't run it unnecessarily. Maybe return a result from ensure_repo() and update_repo() indicating whether doing something was necessary?
https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies.py File ensure_dependencies.py (right): https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:258: Do nothing when the SKIP_DEPENDENCY_UPDATES environment variable is set. On 2017/08/24 11:38:12, Wladimir Palant wrote: > SKIP_DEPENDENCY_UPDATES should be handled implicitly if this only runs on > repository update. Acknowledged. https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:267: # production-dependencies declared, we don't need to run npm and can On 2017/08/24 11:38:12, Wladimir Palant wrote: > Nit: "production dependencies" Acknowledged. https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:279: } On 2017/08/24 11:38:12, Wladimir Palant wrote: > I'd rather have the arguments specified in the actual call rather than in a > separate dictionary. Acknowledged. https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:281: if subprocess.call(['npm', 'outdated'], **subprocess_kwargs) == 0: On 2017/08/24 11:38:12, Wladimir Palant wrote: > Given that `npm outdated` hits the network, I'd rather not call it - just call > `npm install` directly. Besides, this doesn't work correctly anyway, with `npm > outdated` listing outdated development dependencies as well. What do you think about looking for the desired dependencies in a "packages-lock.json", created by "npm install"? this way we would not perform network operations and we would still be able to warn the user about SKIP_DEPENDENCIES_UPDATES only if an update is necessary, but before the actual installation would happen (which would be the same behavior as in ensure_repo / update_repo) https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:292: output, error = npm_process.communicate() On 2017/08/24 11:38:12, Wladimir Palant wrote: > Better use subprocess.check_output() - this will set up stdout and raise errors > automatically. As to stderr output from npm, we should not suppress it IMHO. Acknowledged, it should be check_output. Regarding stderr: npm does not only print errors to stderr (with version 8.x, installed as described in [1]) - e.g. when first run, it gives the note "npm notice created a lockfile as package-lock.json. You should commit this file." via stderr - which imo is not an error) The point of this was to only show an error, if an actual error happened. I could change this, but imho this would cause unnecessary confusion for the users. What do you think about this? [1] https://nodejs.org/en/download/package-manager/ https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:293: logging.info('Node.js: ' + output) On 2017/08/24 11:38:12, Wladimir Palant wrote: > It's npm output, not Node.js. Besides, I doubt that the module tree produced > here is helpful in any way, I'd rather suppress the output. Acknowledged. https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:392: resolve_npm_dependencies(target, vcs) On 2017/08/24 11:38:12, Wladimir Palant wrote: > This will be called every time, not merely when the repository is updated. `npm > install` requires network access and might be a heavy operation, so we shouldn't > run it unnecessarily. Maybe return a result from ensure_repo() and update_repo() > indicating whether doing something was necessary? I thought about this too - but this would require those using ensure_dependencies.py to run 'npm install' manually for the buildtools, if they updated the buildtools dependency from the current revision to a future iteration, where a Node.js dependency was already added (e.g. #5560). When run, ensure_dependencies.py would just update the buildtools-repository and overwrite itself with the updated version, which would then be restarted. After restarting, ensure_dependencies.py would figure that buildtools is up-to-date and hence skip the npm-dependencies. I agree that this could be a corner case (and that the current implementation is a rather heavy implementation) - but this would make sure that running ensure_dependencies.py once results in all desired dependencies being met immediately, without having to do things manually. Additionally, if we agree on what i replied to the "npm outdated" comment, this would also have no network-operation at all (just like ensure_repo / update_repo) What do you think about this?
https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies.py File ensure_dependencies.py (right): https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:281: if subprocess.call(['npm', 'outdated'], **subprocess_kwargs) == 0: On 2017/08/24 13:27:54, tlucas wrote: > What do you think about looking for the desired dependencies in a > "packages-lock.json", created by "npm install"? I don't have a packages-lock.json file. There is probably a reason for that, which we could figure out. Still, it seems to be very much an implementation detail that we shouldn't rely on. IMHO you are overcomplicating. I also tried finding a local approach to determining whether `npm install` would need to do something - surprisingly, there doesn't seem to be one. However, that should be fine. The dependencies cannot change unless the repository state changes. In the SKIP_DEPENDENCY_UPDATES scenario we already produce a warning due to repository not being updated, we don't need another. So we can just run `npm install` blindly whenever changing repository state, this should be good enough and avoid unnecessary complexity. https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:292: output, error = npm_process.communicate() On 2017/08/24 13:27:54, tlucas wrote: > Regarding stderr: npm does not only print errors to stderr (with version 8.x, > installed as described in [1]) - e.g. when first run, it gives the note "npm > notice created a lockfile as package-lock.json. You should commit this file." > via stderr - which imo is not an error) npm documentation could definitely be better, but I found that you can add `--loglevel=warn` parameter. This will hopefully be sufficient to suppress unnecessary messages. https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:392: resolve_npm_dependencies(target, vcs) On 2017/08/24 13:27:54, tlucas wrote: > When run, ensure_dependencies.py would just update the buildtools-repository and > overwrite itself with the updated version, which would then be restarted. After > restarting, ensure_dependencies.py would figure that buildtools is up-to-date > and hence skip the npm-dependencies. The way I see it, this wouldn't normally be an issue - the previous version of ensure_dependencies.py would take care of installing npm dependencies when updating buildtools repository. It's only a problem when updating from ensure_dependencies.py without support of npm dependencies to one with such support. In other words, it's a one-time thing. And it should be solved by updating buildtools dependency in two steps: first we update ensure_dependencies.py, then we add package.json to buildtools. We should add "integration notes" section to issue 5559 description to explain that.
https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies.py File ensure_dependencies.py (right): https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:281: if subprocess.call(['npm', 'outdated'], **subprocess_kwargs) == 0: On 2017/08/25 08:10:42, Wladimir Palant wrote: > I don't have a packages-lock.json file. There is probably a reason for that, And that reason is an outdated npm version (was obvious once I realized that npm version and Node.js version aren't identical). This doesn't affect the conclusions. https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:302: ) It seems that this file should indeed be committed, so please don't ignore it. We'll need to add it in both buildtools and adblockpluscore.
Patch Set 2 * moved resolve_npm_dependecies-call to ensure- /update_repo * adjusted handling npm-output * don't ignore package-lock.json (as it is to be handled by the dependency itself) https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies.py File ensure_dependencies.py (right): https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:258: Do nothing when the SKIP_DEPENDENCY_UPDATES environment variable is set. On 2017/08/24 13:27:54, tlucas wrote: > On 2017/08/24 11:38:12, Wladimir Palant wrote: > > SKIP_DEPENDENCY_UPDATES should be handled implicitly if this only runs on > > repository update. > > Acknowledged. Done. https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:267: # production-dependencies declared, we don't need to run npm and can On 2017/08/24 13:27:54, tlucas wrote: > On 2017/08/24 11:38:12, Wladimir Palant wrote: > > Nit: "production dependencies" > > Acknowledged. Done. https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:279: } On 2017/08/24 13:27:54, tlucas wrote: > On 2017/08/24 11:38:12, Wladimir Palant wrote: > > I'd rather have the arguments specified in the actual call rather than in a > > separate dictionary. > > Acknowledged. Done. https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:281: if subprocess.call(['npm', 'outdated'], **subprocess_kwargs) == 0: On 2017/08/25 08:10:42, Wladimir Palant wrote: > On 2017/08/24 13:27:54, tlucas wrote: > > What do you think about looking for the desired dependencies in a > > "packages-lock.json", created by "npm install"? > > I don't have a packages-lock.json file. There is probably a reason for that, > which we could figure out. Still, it seems to be very much an implementation > detail that we shouldn't rely on. > > IMHO you are overcomplicating. I also tried finding a local approach to > determining whether `npm install` would need to do something - surprisingly, > there doesn't seem to be one. However, that should be fine. The dependencies > cannot change unless the repository state changes. In the > SKIP_DEPENDENCY_UPDATES scenario we already produce a warning due to repository > not being updated, we don't need another. So we can just run `npm install` > blindly whenever changing repository state, this should be good enough and avoid > unnecessary complexity. Fair enough - Done https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:292: output, error = npm_process.communicate() On 2017/08/25 08:10:42, Wladimir Palant wrote: > On 2017/08/24 13:27:54, tlucas wrote: > > Regarding stderr: npm does not only print errors to stderr (with version 8.x, > > installed as described in [1]) - e.g. when first run, it gives the note "npm > > notice created a lockfile as package-lock.json. You should commit this file." > > via stderr - which imo is not an error) > > npm documentation could definitely be better, but I found that you can add > `--loglevel=warn` parameter. This will hopefully be sufficient to suppress > unnecessary messages. Yes, this is sufficient (as discussed in irc, status-updates are ok) - Done. https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:293: logging.info('Node.js: ' + output) On 2017/08/24 13:27:54, tlucas wrote: > On 2017/08/24 11:38:12, Wladimir Palant wrote: > > It's npm output, not Node.js. Besides, I doubt that the module tree produced > > here is helpful in any way, I'd rather suppress the output. > > Acknowledged. Done. https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:302: ) On 2017/08/25 08:16:58, Wladimir Palant wrote: > It seems that this file should indeed be committed, so please don't ignore it. > We'll need to add it in both buildtools and adblockpluscore. Done. https://codereview.adblockplus.org/29526588/diff/29526589/ensure_dependencies... ensure_dependencies.py:392: resolve_npm_dependencies(target, vcs) On 2017/08/25 08:10:42, Wladimir Palant wrote: > On 2017/08/24 13:27:54, tlucas wrote: > > When run, ensure_dependencies.py would just update the buildtools-repository > and > > overwrite itself with the updated version, which would then be restarted. > After > > restarting, ensure_dependencies.py would figure that buildtools is up-to-date > > and hence skip the npm-dependencies. > > The way I see it, this wouldn't normally be an issue - the previous version of > ensure_dependencies.py would take care of installing npm dependencies when > updating buildtools repository. It's only a problem when updating from > ensure_dependencies.py without support of npm dependencies to one with such > support. In other words, it's a one-time thing. And it should be solved by > updating buildtools dependency in two steps: first we update > ensure_dependencies.py, then we add package.json to buildtools. We should add > "integration notes" section to issue 5559 description to explain that. Yes, that sounds reasonable. Done.
https://codereview.adblockplus.org/29526588/diff/29527639/ensure_dependencies.py File ensure_dependencies.py (right): https://codereview.adblockplus.org/29526588/diff/29527639/ensure_dependencies... ensure_dependencies.py:333: resolve_npm_dependencies(target, type) For Mercurial repositories, both cloning and updating might happen. So you will end up resolve_npm_dependencies() twice.
Patch Set 3 Ensured single call of resolve_npm_dependencies() https://codereview.adblockplus.org/29526588/diff/29527639/ensure_dependencies.py File ensure_dependencies.py (right): https://codereview.adblockplus.org/29526588/diff/29527639/ensure_dependencies... ensure_dependencies.py:333: resolve_npm_dependencies(target, type) On 2017/08/25 10:01:59, Wladimir Palant wrote: > For Mercurial repositories, both cloning and updating might happen. So you will > end up resolve_npm_dependencies() twice. Done.
LGTM
Hi Tristan, Looks pretty good apart from some stylistic nits (see below). Cheers, Vasily https://codereview.adblockplus.org/29526588/diff/29527641/ensure_dependencies.py File ensure_dependencies.py (right): https://codereview.adblockplus.org/29526588/diff/29527641/ensure_dependencies... ensure_dependencies.py:266: # bail out early Nit: please add periods to the comments that are complete sentences. PEP8[1] suggests this. [1] https://www.python.org/dev/peps/pep-0008/#comments https://codereview.adblockplus.org/29526588/diff/29527641/ensure_dependencies... ensure_dependencies.py:276: # Make sure Node.js related files / folders are ignored by the VCS in It seems that this comment is redundant given that we've documented the behavior in the docstring and `repo_types[vcs].ignore(...)` seems pretty self-explanatory. https://codereview.adblockplus.org/29526588/diff/29527641/ensure_dependencies... ensure_dependencies.py:278: repo_types[vcs].ignore( Wouldn't this be more readable as one line?
Patch Set 4 Removing an obsolete comment, improving readability. Good morning guys, please have another look at this patch. Best, Tristan https://codereview.adblockplus.org/29526588/diff/29527641/ensure_dependencies.py File ensure_dependencies.py (right): https://codereview.adblockplus.org/29526588/diff/29527641/ensure_dependencies... ensure_dependencies.py:266: # bail out early On 2017/08/25 18:37:40, Vasily Kuznetsov wrote: > Nit: please add periods to the comments that are complete sentences. PEP8[1] > suggests this. > > [1] https://www.python.org/dev/peps/pep-0008/#comments Done. https://codereview.adblockplus.org/29526588/diff/29527641/ensure_dependencies... ensure_dependencies.py:276: # Make sure Node.js related files / folders are ignored by the VCS in On 2017/08/25 18:37:40, Vasily Kuznetsov wrote: > It seems that this comment is redundant given that we've documented the behavior > in the docstring and `repo_types[vcs].ignore(...)` seems pretty > self-explanatory. Done. https://codereview.adblockplus.org/29526588/diff/29527641/ensure_dependencies... ensure_dependencies.py:278: repo_types[vcs].ignore( On 2017/08/25 18:37:40, Vasily Kuznetsov wrote: > Wouldn't this be more readable as one line? Done.
Still LGTM.
LGTM 
|
