Opened 3 years ago

Closed 3 years ago

#4540 closed change (fixed)

Update createNightlies script to support creating dev builds from bookmarked branches

Reported by: kzar Assignee: jsonesen
Priority: P2 Milestone:
Module: Sitescripts Keywords:
Cc: trev, kvas, sebastian, oleksandr Blocked By:
Blocking: #4551 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29358368/

Description (last modified by kzar)

Background

We are moving Safari support for adblockpluschrome into a separate safari bookmark. That will allow us to remove Safari specific code, workarounds and hacks from the main code. We plan to then make the minimum required changes to the code under the safari bookmark to keep things working.

We also plan to add Edge support for adblockpluschrome, until it is ready to be merged into the master bookmark we want to develop that under a separate edge bookmark too.

For both the safari and edge bookmarks we need to be able to control the builds properly:

Changes to the safari or edge branches should only trigger a development build for the relevant platform. Changes to the master branch should only trigger a development build for the other supported platforms.

What to change

Add a configuration option to set the branch to be used for development builds, e.g. setting abpsafari_bookmark = safari should make sitescripts.extensions.bin.createNightlies check out the safari bookmark. If no such configuration option is given, the default should be master (as opposed as default that we use right now).

Change History (17)

comment:1 Changed 3 years ago by trev

  • Component changed from Build-and-Release-Tools to Sitescripts
  • Description modified (diff)
  • Summary changed from Update buildtools to support boomarks for dev and release builds to Update createNightlies script to support creating dev builds from bookmarked branches

comment:2 Changed 3 years ago by kzar

Vasily any chance you could prioritise this issue? Once it's done we can set up the safari bookmark and begin stripping out a whole bunch of code from master. I'm hoping the next adblockpluschrome release will be the last one to include Safari.

comment:3 Changed 3 years ago by sebastian

  • Blocking 4028 removed
  • Ready set

I don't see why #4028 depends on this issue. Although for Microsoft Edge we might need both for now, implementing the packager and creating devbuilds out of a bookmark, can be tackled separately.

comment:4 Changed 3 years ago by kzar

  • Blocking 4551 added

comment:5 Changed 3 years ago by jsonesen

Hi,

I will take this ticket and prioritize it for getting into review today. One question I have is regarding the nightlies config file. Can I have an example of that to use for testing? I want to add tests for this change.

comment:6 Changed 3 years ago by kzar

Awesome, thanks Jon. Unless I'm missing something this will use the global sitescripts config. If so there's an example in the sitescripts repository.

comment:7 Changed 3 years ago by kzar

Ah I think I understand now, you mean the nightliesData=%(root)s/data/nightlies file? If so I'm guessing Wladimir would be a good person to ask for an example of that, or someone else with server access.

comment:8 Changed 3 years ago by jsonesen

  • Owner set to jsonesen

comment:9 Changed 3 years ago by jsonesen

The config appears to be generated at the time of the nightly creation so i'lll be okay without it. My plan for the implementation is as follows:

Add attribute to the Nightlybuild objects init function which will look for the presence of a config parameter in the extensions section of the config that starts with the platform type (in the code it is self.config.type) and ends with 'branch' for example: safari_branch = safari if the conf item is found it will do self.branch = self.config.get('extensions', self.config.type+'_branch') or something similar then it will use that as the branch in all mercurial calls throughout the methods that call it.

In the description you preface the platform with abp which I can also do, is that preferable?

comment:10 Changed 3 years ago by jsonesen

for now I will add the abp prefix

comment:11 Changed 3 years ago by kzar

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:12 Changed 3 years ago by kzar

  • Description modified (diff)

comment:13 follow-up: Changed 3 years ago by trev

@kzar: Why did you rename it from abpsafari_branch to abpsafari_bookmark? Bookmarks are merely one branching mechanism, why should we hardcode implementation details here? In fact, this should work fine with any revision identifier so I would be fine with abpsafari_revision as well.

comment:14 in reply to: ↑ 13 Changed 3 years ago by kzar

Replying to trev:

@kzar: Why did you rename it from abpsafari_branch to abpsafari_bookmark? Bookmarks are merely one branching mechanism, why should we hardcode implementation details here? In fact, this should work fine with any revision identifier so I would be fine with abpsafari_revision as well.

Well because I thought it was clearer, branches in Mercurial are something else. But sure you're right as well and I don't insist.

comment:15 Changed 3 years ago by jsonesen

Ill just call it revision then

Last edited 3 years ago by jsonesen (previous) (diff)

comment:16 Changed 3 years ago by abpbot

comment:17 Changed 3 years ago by kzar

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.