Issue 29345508: Issue 4098 - Get rid of special build setup for development builds (Closed)
|
Created:
June 1, 2016, 2:47 p.m. by Wladimir Palant Modified:
June 6, 2016, 12:58 p.m. Reviewers:
Vasily Kuznetsov Visibility:
Public. |
DescriptionThe patch is complete now. It replaces direct calls to buildtools with subprocess calls where possible and adds setting SPIDERMONKEY_BINARY environment variable.
Repository: hg.adblockplus.org/sitescripts
Patch Set 1 #
Total comments: 18
Patch Set 2 : Addressed comments #
Total comments: 3
Patch Set 3 : Added spiderMonkeyPath config setting #Patch Set 4 : Improved ensure_dependencies call #
MessagesTotal messages: 7
https://codereview.adblockplus.org/29345508/diff/29345509/.sitescripts.example File .sitescripts.example (left): https://codereview.adblockplus.org/29345508/diff/29345509/.sitescripts.exampl... .sitescripts.example:96: urlfixer_galleryID=url-fixer I removed deobfuscator and urlfixer because listing them here is pointless - neither is hosted on adblockplus.org, even though we still produce development builds for the latter. https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:72: self.buildNum = None With these changes, self.revision is a revision hash like 814673439cbc, only used to recognize whether there are changes in the repository. self.buildNum on the other hand is the numerical revision ID, what we had in self.revision before. Reason is that we cannot retrieve a numerical revision ID for a remote repository, self.buildNum has to be populated after cloning the repository. Side-effect of this change is that all builds will be recreated after this lands (latestRevision won't match) but that's not really an issue. https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:157: import buildtools.packagerGecko as packager We are still using latest buildtools revision to read metadata here. Eventually we probably want to create a command line interface to retrieve this data, it's not such a big issue however.
In general, looks good. I have a few minor questions/suggestions below. I haven't tested it, for the reasons you've mentioned and because I'm not quite sure how to recreate the whole nightlies building environment. Let me know if you would like that. Cheers, Vasily https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:72: self.buildNum = None On 2016/06/01 14:57:12, Wladimir Palant wrote: > With these changes, self.revision is a revision hash like 814673439cbc, only > used to recognize whether there are changes in the repository. self.buildNum on > the other hand is the numerical revision ID, what we had in self.revision > before. Reason is that we cannot retrieve a numerical revision ID for a remote > repository, self.buildNum has to be populated after cloning the repository. > Side-effect of this change is that all builds will be recreated after this lands > (latestRevision won't match) but that's not really an issue. Acknowledged. https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:85: 'hg', 'id', '-i', '-r', 'default', '--config', 'defaults.id=', This `--config defaults.id=` is there to make sure there are no additional options to `hg id` coming from [defaults] section of .hgrc, right? Since [defaults] section seems to be deprecated, I was wandering, are we using it somewhere on the build servers? And if not, is this override needed? https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:126: if os.path.isfile(command[0]): Are we sure that `ensure_dependecies.py` is always executable. Maybe we should check if it is or do ['python', '....../ensure_dependencies.py', '-q'] instead to be more robust? https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:157: import buildtools.packagerGecko as packager On 2016/06/01 14:57:12, Wladimir Palant wrote: > We are still using latest buildtools revision to read metadata here. Eventually > we probably want to create a command line interface to retrieve this data, it's > not such a big issue however. Acknowledged. https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:160: self.version = packager.getBuildVersion(self.tempdir, metadata, False, self.buildNum) This line is too long, although of course, it's not the fault of this change (and it's definitely not the only one in this file). But perhaps you could wrap it (and a few other changed long lines) while it's being changed anyway. https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:310: buildCommand = [ A nice side effect that we got rid of this branching! https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensi... File sitescripts/extensions/utils.py (left): https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensi... sitescripts/extensions/utils.py:136: buildRepository = _defineProperty('buildRepository') I see you removed this variable from the config. Was it used before, I don't see any relevant changes removing the usage?
This patch has been finalized and tested now. All blockers landed so we are good to go. https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:85: 'hg', 'id', '-i', '-r', 'default', '--config', 'defaults.id=', On 2016/06/01 17:14:15, Vasily Kuznetsov wrote: > This `--config defaults.id=` is there to make sure there are no additional > options to `hg id` coming from [defaults] section of .hgrc, right? Since > [defaults] section seems to be deprecated, I was wandering, are we using it > somewhere on the build servers? And if not, is this override needed? "Breaking automation is only one of the reason to not use defaults." :) No, we don't use defaults on our servers. This was added in order to simplify testing, because local machines (mine in particular) tend to have defaults. https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:126: if os.path.isfile(command[0]): On 2016/06/01 17:14:15, Vasily Kuznetsov wrote: > Are we sure that `ensure_dependecies.py` is always executable. Maybe we should > check if it is or do ['python', '....../ensure_dependencies.py', '-q'] instead > to be more robust? You are right, build.py calls Python explicitly so it doesn't have to be executable. Fixed. https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:160: self.version = packager.getBuildVersion(self.tempdir, metadata, False, self.buildNum) On 2016/06/01 17:14:15, Vasily Kuznetsov wrote: > This line is too long, although of course, it's not the fault of this change > (and it's definitely not the only one in this file). But perhaps you could wrap > it (and a few other changed long lines) while it's being changed anyway. This add too many unrelated changes for my taste but well - done. https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:310: buildCommand = [ On 2016/06/01 17:14:15, Vasily Kuznetsov wrote: > A nice side effect that we got rid of this branching! It will come back, as soon as we remove signing support for Gecko :) https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensi... File sitescripts/extensions/utils.py (left): https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensi... sitescripts/extensions/utils.py:136: buildRepository = _defineProperty('buildRepository') On 2016/06/01 17:14:15, Vasily Kuznetsov wrote: > I see you removed this variable from the config. Was it used before, I don't see > any relevant changes removing the usage? No, it doesn't look like it was actually used, not sure whether it ever was.
Looks good. Just one small thing (see below). https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:85: 'hg', 'id', '-i', '-r', 'default', '--config', 'defaults.id=', On 2016/06/03 14:14:05, Wladimir Palant wrote: > On 2016/06/01 17:14:15, Vasily Kuznetsov wrote: > > This `--config defaults.id=` is there to make sure there are no additional > > options to `hg id` coming from [defaults] section of .hgrc, right? Since > > [defaults] section seems to be deprecated, I was wandering, are we using it > > somewhere on the build servers? And if not, is this override needed? > > "Breaking automation is only one of the reason to not use defaults." :) > > No, we don't use defaults on our servers. This was added in order to simplify > testing, because local machines (mine in particular) tend to have defaults. Acknowledged. https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:310: buildCommand = [ On 2016/06/03 14:14:04, Wladimir Palant wrote: > On 2016/06/01 17:14:15, Vasily Kuznetsov wrote: > > A nice side effect that we got rid of this branching! > > It will come back, as soon as we remove signing support for Gecko :) :) https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensi... File sitescripts/extensions/utils.py (left): https://codereview.adblockplus.org/29345508/diff/29345509/sitescripts/extensi... sitescripts/extensions/utils.py:136: buildRepository = _defineProperty('buildRepository') On 2016/06/03 14:14:06, Wladimir Palant wrote: > On 2016/06/01 17:14:15, Vasily Kuznetsov wrote: > > I see you removed this variable from the config. Was it used before, I don't > see > > any relevant changes removing the usage? > > No, it doesn't look like it was actually used, not sure whether it ever was. Acknowledged. https://codereview.adblockplus.org/29345508/diff/29345599/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29345508/diff/29345599/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:129: if os.path.isfile(command[0]): Should not this become `command[1]` now that we've inserted python as the first item of the list?
https://codereview.adblockplus.org/29345508/diff/29345599/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29345508/diff/29345599/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:129: if os.path.isfile(command[0]): On 2016/06/03 16:04:49, Vasily Kuznetsov wrote: > Should not this become `command[1]` now that we've inserted python as the first > item of the list? Ouch, you are right. In fact, I had a better idea in order to make this code less fragile.
LGTM https://codereview.adblockplus.org/29345508/diff/29345599/sitescripts/extensi... File sitescripts/extensions/bin/createNightlies.py (right): https://codereview.adblockplus.org/29345508/diff/29345599/sitescripts/extensi... sitescripts/extensions/bin/createNightlies.py:129: if os.path.isfile(command[0]): On 2016/06/06 11:47:46, Wladimir Palant wrote: > On 2016/06/03 16:04:49, Vasily Kuznetsov wrote: > > Should not this become `command[1]` now that we've inserted python as the > first > > item of the list? > > Ouch, you are right. In fact, I had a better idea in order to make this code > less fragile. Lovely! 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
