Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29321227: Issue 2735 - Use ensure_dependencies.py in adblockplusie (Closed)

Created:
June 30, 2015, 12:29 a.m. by Oleksandr
Modified:
July 12, 2015, 9:12 p.m.
Reviewers:
sergei, Felix Dahlke, Eric
Visibility:
Public.

Description

I don't have a strong opinion on this, but I have actually removed the buildtools directory from adblockplusie, since we can just use libadblockplus/adblockplus/buildtools.

Patch Set 1 : #

Total comments: 5

Patch Set 2 : Update libadblockplus version. Use the same version of ensure_dependencies.py as in buildtools #

Unified diffs Side-by-side diffs Delta from patch set Stats (+319 lines, -5 lines) Patch
M README.md View 2 chunks +14 lines, -5 lines 0 comments Download
M createsolution.bat View 1 chunk +3 lines, -0 lines 0 comments Download
A dependencies View 1 1 chunk +3 lines, -0 lines 0 comments Download
A ensure_dependencies.py View 1 1 chunk +299 lines, -0 lines 0 comments Download

Messages

Total messages: 9
Oleksandr
June 30, 2015, 12:30 a.m. (2015-06-30 00:30:51 UTC) #1
sergei
https://codereview.adblockplus.org/29321227/diff/29321235/createsolution.bat File createsolution.bat (right): https://codereview.adblockplus.org/29321227/diff/29321235/createsolution.bat#newcode9 createsolution.bat:9: python ensure_dependencies.py It means that git users won't be ...
June 30, 2015, 9:51 a.m. (2015-06-30 09:51:06 UTC) #2
sergei
LGTM however I will need to figure out an issue with the availability of git.
June 30, 2015, 10:49 a.m. (2015-06-30 10:49:12 UTC) #3
Eric
We should be running ensure_dependencies at the beginning of build_release.py, in order to allow weaker ...
July 1, 2015, 4:58 p.m. (2015-07-01 16:58:27 UTC) #4
Felix Dahlke
I agree that integrating this call into createsolution.bat is better than to integrate it into ...
July 2, 2015, 10:02 a.m. (2015-07-02 10:02:06 UTC) #5
Oleksandr
July 2, 2015, 10:23 a.m. (2015-07-02 10:23:30 UTC) #6
sergei
LGTM. However, I would add or change the commit message. /buildtools are removed because we ...
July 2, 2015, 10:51 a.m. (2015-07-02 10:51:06 UTC) #7
Felix Dahlke
LGTM, BUT, I think this review/commit should be linked to https://issues.adblockplus.org/ticket/2732 which is about the ...
July 2, 2015, 1:17 p.m. (2015-07-02 13:17:37 UTC) #8
Eric
July 4, 2015, 4:46 p.m. (2015-07-04 16:46:46 UTC) #9
LGTM

https://codereview.adblockplus.org/29321227/diff/29321235/createsolution.bat
File createsolution.bat (right):

https://codereview.adblockplus.org/29321227/diff/29321235/createsolution.bat#...
createsolution.bat:9: python ensure_dependencies.py
On 2015/07/02 10:02:06, Felix Dahlke wrote:
> The script does nothing if you're already on the
> desired revision (i.e. the one from the dependencies file).

OK. That's not obvious from the code. (I didn't read it long enough to determine
this.) A description of this behavior really ought to be part of the
documentation for the script.

Powered by Google App Engine
This is Rietveld