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

Issue 10329009: Changes to convert_js.py to support gyp restrictions (Closed)

Created:
April 18, 2013, 6:56 p.m. by Eric
Modified:
April 20, 2013, 1:18 p.m.
Visibility:
Public.

Description

Changes to convert_js.py to support gyp restrictions. This needs testing on Linux and MacOS, not just looking at the text. The changes to libadblockplus.gyp were extracted by hand from my personal modifications for Windows (on an as-yet secret branch on my local repository), because libadblockplus.gyp as it is now is still failing spectacularly on Windows. It does work, however, on my modified version, which has been tested only on Windows.

Patch Set 1 #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+30 lines, -22 lines) Patch
M convert_js.py View 3 chunks +15 lines, -17 lines 0 comments Download
M libadblockplus.gyp View 2 chunks +15 lines, -5 lines 4 comments Download

Messages

Total messages: 3
Eric
I should have hit this button on the review UI yesterday, apparently.
April 19, 2013, 12:52 p.m. (2013-04-19 12:52:44 UTC) #1
Wladimir Palant
I integrated a bunch of your changes already to avoid further bitrot, commented on the ...
April 19, 2013, 1:31 p.m. (2013-04-19 13:31:19 UTC) #2
Eric
April 19, 2013, 1:55 p.m. (2013-04-19 13:55:49 UTC) #3
http://codereview.adblockplus.org/10329009/diff/1/libadblockplus.gyp
File libadblockplus.gyp (right):

http://codereview.adblockplus.org/10329009/diff/1/libadblockplus.gyp#newcode3
libadblockplus.gyp:3: 'python%': 'python',
On 2013/04/19 13:31:19, Wladimir Palant wrote:
> You might want to explain briefly why you need Python to be parametrized
> (probably a comment in the build file).

Comment added in my working version. It'll be present the next time I offer this
change.

http://codereview.adblockplus.org/10329009/diff/1/libadblockplus.gyp#newcode18
libadblockplus.gyp:18: 'msvs_cygwin_shell': 0,
On 2013/04/19 13:31:19, Wladimir Palant wrote:
> Wouldn't we want that setting for all targets?

We would, in fact, but the msvs generator does not support that specification.
It only looks for this option here; there's no global setting for it.

Similarly for msvs_quote_cmd below. That one is only looked for on the action
itself.

Yeah. I know.

Powered by Google App Engine
This is Rietveld