Issue 29901591: Issue 7020 - publish gecko with Node.js (Closed)
|
Created:
Oct. 4, 2018, 3:21 p.m. by tlucas Modified:
Oct. 17, 2018, 6:48 a.m. Base URL:
https://gitlab.com/eyeo/adblockplus/adblockpluschrome/tree/6323e74d580026e3bd1e8e631fcddaf0bbaa34df Visibility:
Public. |
DescriptionIssue 7020 - publish gecko with Node.js
Patch Set 1 #
Total comments: 17
Patch Set 2 : #
Total comments: 10
Patch Set 3 : #
MessagesTotal messages: 8
https://codereview.adblockplus.org/29901591/diff/29901592/automation/build.js File automation/build.js (left): https://codereview.adblockplus.org/29901591/diff/29901592/automation/build.js... automation/build.js:1: /* For some reason, rietveld recognized this as a change for ext/devtools.js -> In a `git show` it's clearly a new file: index 0000000..983f065 --- /dev/null +++ b/automation/build.js @@ -0,0 +1,26 @@ +/* + * This file is part of Adblock Plus <https://adblockplus.org/>, ...
I'd like to suggest a different file structure: build/publish.js build/target/gecko.js Also it seems almost all code in gecko.js except for the call to signAddon() will be the same for all other build targets. So I wonder whether this is the right abstraction you got here. https://codereview.adblockplus.org/29901591/diff/29901592/automation/build.js File automation/build.js (left): https://codereview.adblockplus.org/29901591/diff/29901592/automation/build.js... automation/build.js:1: /* On 2018/10/04 15:27:07, tlucas wrote: > For some reason, rietveld recognized this as a change for ext/devtools.js -> In > a `git show` it's clearly a new file: > > index 0000000..983f065 > --- /dev/null > +++ b/automation/build.js > @@ -0,0 +1,26 @@ > +/* > + * This file is part of Adblock Plus <https://adblockplus.org/>, > > ... Welcome to Git! There is no information like file movement in the internal data structures of Git (unlike Mercurial). Commands like "git diff" just try to be smarter than you and guess what file has been moved where. Though, it's arguably weird that it does so if ext/devtools.js still exists. https://codereview.adblockplus.org/29901591/diff/29901592/automation/build.js File automation/build.js (right): https://codereview.adblockplus.org/29901591/diff/29901592/automation/build.js... automation/build.js:18: /* eslint-env node */ If we put these scripts into a separate directory we might rather add an .eslintrc.json (similar to the on we have in the "test" directory). https://codereview.adblockplus.org/29901591/diff/29901592/automation/build.js... automation/build.js:24: let platform = process.argv[2]; This temporary variable seems unnecessary. You could inline process.argv[2] below within the template string. https://codereview.adblockplus.org/29901591/diff/29901592/automation/builds/g... File automation/builds/gecko.js (right): https://codereview.adblockplus.org/29901591/diff/29901592/automation/builds/g... automation/builds/gecko.js:27: let buildnum = undefined; Do these really need to be global variables? Why? https://codereview.adblockplus.org/29901591/diff/29901592/automation/builds/g... automation/builds/gecko.js:31: fs.readFileSync(path.resolve(process.env.AMO_OAUTH_CREDENTIALS))); I'm not sure if using sync APIs is justified here. https://codereview.adblockplus.org/29901591/diff/29901592/automation/builds/g... automation/builds/gecko.js:46: else resolve(stdout.replace(/(\r\n|\n|\r)/gm, "")); Nit: Please a new line after the else statement. https://codereview.adblockplus.org/29901591/diff/29901592/automation/builds/g... automation/builds/gecko.js:46: else resolve(stdout.replace(/(\r\n|\n|\r)/gm, "")); Do you really want to remove every new line (and if so this regexp can be much simpler: /[\r\n]/gm), or do you rather want to just trim the output? https://codereview.adblockplus.org/29901591/diff/29901592/automation/builds/g... automation/builds/gecko.js:74: `bash -c "python build.py build -t gecko -b \ Calling the command through bash seems unnecessary here. We can expect the build tooling being run in a UNIX-like environment. The situation in the test runner is different, as on Windows it must be executed on native Windows in order to run the tests against the Windows version of the browsers. https://codereview.adblockplus.org/29901591/diff/29901592/automation/builds/g... automation/builds/gecko.js:88: process.exit(1); Is there any error we can log in this case?
Patch Set 2: * Rename Issue * Adding top-level argument parsing. * Use parsed arguments, instead of environment variables * Read necessary information from package-filename, rather than invoking multiple inline python-scripts. * Adhere to packages already being build by https://codereview.adblockplus.org/29904564/ https://codereview.adblockplus.org/29901591/diff/29901592/automation/build.js File automation/build.js (right): https://codereview.adblockplus.org/29901591/diff/29901592/automation/build.js... automation/build.js:18: /* eslint-env node */ On 2018/10/04 19:56:52, Sebastian Noack wrote: > If we put these scripts into a separate directory we might rather add an > .eslintrc.json (similar to the on we have in the "test" directory). Done. https://codereview.adblockplus.org/29901591/diff/29901592/automation/build.js... automation/build.js:24: let platform = process.argv[2]; On 2018/10/04 19:56:52, Sebastian Noack wrote: > This temporary variable seems unnecessary. You could inline process.argv[2] > below within the template string. Not relevant anymore. https://codereview.adblockplus.org/29901591/diff/29901592/automation/builds/g... File automation/builds/gecko.js (right): https://codereview.adblockplus.org/29901591/diff/29901592/automation/builds/g... automation/builds/gecko.js:27: let buildnum = undefined; On 2018/10/04 19:56:53, Sebastian Noack wrote: > Do these really need to be global variables? Why? Done. https://codereview.adblockplus.org/29901591/diff/29901592/automation/builds/g... automation/builds/gecko.js:31: fs.readFileSync(path.resolve(process.env.AMO_OAUTH_CREDENTIALS))); On 2018/10/04 19:56:53, Sebastian Noack wrote: > I'm not sure if using sync APIs is justified here. I "promisified" fs.readFile(), so that we can wait for it in "Promise.all()". https://codereview.adblockplus.org/29901591/diff/29901592/automation/builds/g... automation/builds/gecko.js:46: else resolve(stdout.replace(/(\r\n|\n|\r)/gm, "")); On 2018/10/04 19:56:53, Sebastian Noack wrote: > Do you really want to remove every new line (and if so this regexp can be much > simpler: /[\r\n]/gm), or do you rather want to just trim the output? It was trimming. Done. https://codereview.adblockplus.org/29901591/diff/29901592/automation/builds/g... automation/builds/gecko.js:74: `bash -c "python build.py build -t gecko -b \ On 2018/10/04 19:56:53, Sebastian Noack wrote: > Calling the command through bash seems unnecessary here. We can expect the build > tooling being run in a UNIX-like environment. The situation in the test runner > is different, as on Windows it must be executed on native Windows in order to > run the tests against the Windows version of the browsers. Done. https://codereview.adblockplus.org/29901591/diff/29901592/automation/builds/g... automation/builds/gecko.js:88: process.exit(1); On 2018/10/04 19:56:52, Sebastian Noack wrote: > Is there any error we can log in this case? signAddon itself prints the reason to the console, e.g.: Server response: Version already exists. Latest version is: ... (status: 409)
https://codereview.adblockplus.org/29901591/diff/29911573/automation/publish.js File automation/publish.js (right): https://codereview.adblockplus.org/29901591/diff/29911573/automation/publish.... automation/publish.js:44: platforms[args.platform_name].run(args); I guess if the platform doesn't exist we'll just get an exception printed? I wonder if instead we should print a list of the available platforms - that might be useful if the user typed "firefox" instead of "gecko" or something like that. https://codereview.adblockplus.org/29901591/diff/29911573/automation/target/g... File automation/target/gecko.js (right): https://codereview.adblockplus.org/29901591/diff/29911573/automation/target/g... automation/target/gecko.js:42: "python -c \"" + Maybe we should parse the file using JavaScript instead? https://codereview.adblockplus.org/29901591/diff/29911573/automation/target/g... automation/target/gecko.js:81: process.exit(1); Should we instead throw an exception here, so it's caught and logged below?
https://codereview.adblockplus.org/29901591/diff/29911573/automation/publish.js File automation/publish.js (right): https://codereview.adblockplus.org/29901591/diff/29911573/automation/publish.... automation/publish.js:44: platforms[args.platform_name].run(args); On 2018/10/15 12:42:58, kzar wrote: > I guess if the platform doesn't exist we'll just get an exception printed? I > wonder if instead we should print a list of the available platforms - that might > be useful if the user typed "firefox" instead of "gecko" or something like that. That's what argparse does for you, e.g.: $ npm run publish -- firefox > adblockpluschrome@ publish /home/tlucas/abp/gitrepo/adblockpluschrome > node automation/publish.js "firefox" usage: publish.js [-h] {gecko} ... publish.js: error: argument "platform_name": Invalid choice: firefox (choose from [gecko]) https://codereview.adblockplus.org/29901591/diff/29911573/automation/target/g... File automation/target/gecko.js (right): https://codereview.adblockplus.org/29901591/diff/29911573/automation/target/g... automation/target/gecko.js:42: "python -c \"" + On 2018/10/15 12:42:58, kzar wrote: > Maybe we should parse the file using JavaScript instead? Originally, we would have to support inheritance in .ini-format files to accomplish that. Unfortunately, no .ini-parsers for nodejs support that feature (and in the buildtools, we added it ourselves) - and for the time being Sebastian and i agreed, that "a little inline python" is acceptable, before we implement the full feature in JS. https://codereview.adblockplus.org/29901591/diff/29911573/automation/target/g... automation/target/gecko.js:81: process.exit(1); On 2018/10/15 12:42:58, kzar wrote: > Should we instead throw an exception here, so it's caught and logged below? Sebastian also asked if we could log something here: https://codereview.adblockplus.org/29901591/diff/29901592/automation/builds/g... -> the last comment section.
https://codereview.adblockplus.org/29901591/diff/29911573/automation/publish.js File automation/publish.js (right): https://codereview.adblockplus.org/29901591/diff/29911573/automation/publish.... automation/publish.js:44: platforms[args.platform_name].run(args); On 2018/10/15 14:12:39, tlucas wrote: > On 2018/10/15 12:42:58, kzar wrote: > > I guess if the platform doesn't exist we'll just get an exception printed? I > > wonder if instead we should print a list of the available platforms - that > might > > be useful if the user typed "firefox" instead of "gecko" or something like > that. > > That's what argparse does for you, e.g.: > > $ npm run publish -- firefox > > > adblockpluschrome@ publish /home/tlucas/abp/gitrepo/adblockpluschrome > > node automation/publish.js "firefox" > > usage: publish.js [-h] {gecko} ... > publish.js: error: argument "platform_name": Invalid choice: firefox (choose > from [gecko]) Cool! https://codereview.adblockplus.org/29901591/diff/29911573/automation/target/g... File automation/target/gecko.js (right): https://codereview.adblockplus.org/29901591/diff/29911573/automation/target/g... automation/target/gecko.js:42: "python -c \"" + On 2018/10/15 14:12:39, tlucas wrote: > On 2018/10/15 12:42:58, kzar wrote: > > Maybe we should parse the file using JavaScript instead? > > Originally, we would have to support inheritance in .ini-format files to > accomplish that. Unfortunately, no .ini-parsers for nodejs support that feature > (and in the buildtools, we added it ourselves) - and for the time being > Sebastian and i agreed, that "a little inline python" is acceptable, before we > implement the full feature in JS. Alright, fair enough. Personally, I'd vote for just switching it to JSON or sometihng that's easier to parse from Nodejs, but I guess we can worry about that later! https://codereview.adblockplus.org/29901591/diff/29911573/automation/target/g... automation/target/gecko.js:81: process.exit(1); On 2018/10/15 14:12:39, tlucas wrote: > On 2018/10/15 12:42:58, kzar wrote: > > Should we instead throw an exception here, so it's caught and logged below? > > Sebastian also asked if we could log something here: > > https://codereview.adblockplus.org/29901591/diff/29901592/automation/builds/g... > -> the last comment section. I see, that's a little misleading but it's not your fault, but the fault of this sign-addon library. Perhaps add a small comment above process.exit(1) to mention that signAddon logs the failure reason to the console?
Patch Set 3 * Adding a comment about signAddon's logging behaviour https://codereview.adblockplus.org/29901591/diff/29911573/automation/target/g... File automation/target/gecko.js (right): https://codereview.adblockplus.org/29901591/diff/29911573/automation/target/g... automation/target/gecko.js:81: process.exit(1); On 2018/10/15 14:46:10, kzar wrote: > On 2018/10/15 14:12:39, tlucas wrote: > > On 2018/10/15 12:42:58, kzar wrote: > > > Should we instead throw an exception here, so it's caught and logged below? > > > > Sebastian also asked if we could log something here: > > > > > https://codereview.adblockplus.org/29901591/diff/29901592/automation/builds/g... > > -> the last comment section. > > I see, that's a little misleading but it's not your fault, but the fault of this > sign-addon library. Perhaps add a small comment above process.exit(1) to mention > that signAddon logs the failure reason to the console? Done.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
