Opened 4 years ago

Closed 2 years ago

#4105 closed change (fixed)

[build] Make it possible to build arbitrary ABB revisions

Reported by: fhd Assignee: rjeschke
Priority: P3 Milestone: Adblock-Browser-for-Android-1.3.3
Module: Adblock-Browser-for-Android Keywords:
Cc: kvas, rjeschke, diegocarloslima Blocked By:
Blocking: Platform: Adblock Browser for Android
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29552627/
https://codereview.adblockplus.org/29631219/

Description (last modified by rjeschke)

Background

adblockbrowser-build is currently specifying the revision of Adblock Browser to build in its dependencies file. That's a suboptimal way of linking the two repositories together, since that file needs to be updated every time a commit to adblockbrowser is made in order to support build automation.

There are multiple possible solutions to this:

  1. Automatically update the dependencies file in adblockbrowser-build whenever a commit is pushed to the adblockbrowser repository.
  2. Reverse the dependency and have the adblockbrowser repository depend on the adblockbrowser-build repository. The latter changes less often, and explicit updates should be realistic there.
  3. Don't use ensure_dependencies.py for this, but specify the revision/branch of adblockbrowser to build at runtime.

To my knowledge, the third option is how Mozilla tackles this. It's quite straightforward if we always build the latest revision, but in order to build older versions we'd still need some sort of revision mapping.

As discussed below, option 2 makes most sense to us. There is no strong reason why we would add this as a dependency and not integrate it into the repository, so we should do that.

After another quite lengthy process and some discussions we (fhd and rjeschke) agreed on using a different approach:

What to change

  1. Add the contents of the adblockbrowser-build repository to the adblockbrowser repository, into a new directory called abb-build. This is done using a single commit, discarding the history as there's no good way to mix the two repositories.
  1. Adjust abb-build/build.py and abb-build/adblockbrowser-cfg.py to find the Adblock Browser code in its parent directory, remove adblockbrowser from abb-build/dependencies.
  1. The adblockbrowser-build repository will be marked deprecated, set read-only and is from now on only used for older builds.

Change History (22)

comment:1 Changed 4 years ago by fhd

  • Owner set to fhd

Assigning to myself since I've already done some work here a while ago. Never got around to create an issue until now :P

Didn't mark this as Ready though, I'm not 100% convinced my preferred solution is the best one. Maybe we should take a closer look at option 2 above for simplicity's sake. René, Vasily - let me know what you think please.

comment:2 Changed 4 years ago by fhd

  • Summary changed from Make it possible to build arbitrary ABB revisions with adblockbrowser-build to [build] Make it possible to build arbitrary ABB revisions

comment:3 Changed 4 years ago by kvas

I don't really know much about the adblockbrowser build process, but I like option 2. It seems like it just makes the whole thing simpler: update to the revision/branch/tag that you want to build, run the build (that does ensure_dependencies internally) and it all works without any extra magic.

comment:4 follow-up: Changed 4 years ago by fhd

Yeah, my thinking, it just seems a bit ugly is all :D Here's how that would work in practice:

  1. Check out the revision of adblockbrowser you want to build
  2. Run ensure_dependencies.py
  3. cd into adblockbrowser-build
  4. Run build.py (which would have to realise it's supposed to find ABB as its own parent directory, either that or we make the path configurable)

(If we go down that route, we should probably think about not having adblockbrowser-build be a separate project in the long run, but for now a dependency should be a good start.)

René, what do you think?

comment:5 Changed 4 years ago by rjeschke

The thing here is that Mozilla more or less changes the Android build dependencies with every major revision. They now (44.0.2), somewhat, settled on a nicer dependency management solution (using the m2repository for special version), still this isn't true for earlier builds. So, just checking out the revision doesn't help you with e.g. downgrading the support library and stuff (but on the other hand, that's a different issue).

Also, different versions of Fennec require different mozconfigs, so we definitely need a link between the adblockbrowser revision to build and the adblockbrowser-build revision.

It seems, that the easiest way to couple those two would be, to have adblockbrowser-build either as a dependency (via ensure_dependencies.py) or as part of the adblockbrowser repository itself (i.e. no separate repository).

Last edited 4 years ago by rjeschke (previous) (diff)

comment:6 in reply to: ↑ 4 Changed 4 years ago by kvas

Here's how that would work in practice:

  1. Check out the revision of adblockbrowser you want to build
  2. Run ensure_dependencies.py
  3. cd into adblockbrowser-build
  4. Run build.py (which would have to realise it's supposed to find ABB as its own parent directory, either that or we make the path configurable)

I guess steps 2-4 could be automated with some script that lives in in the root of adblockbrowser. Maybe we could also skip step 3 and make build.py expect to find ABB in current working directory. This would be less ugly :)

comment:7 Changed 4 years ago by fhd

Good point René, with all the Android baggage, building old revisions is non-trivial to begin with, and shouldn't be a huge concern for this here.

Regarding option 2: Yes, at the end of the day we could just integrate adblockbrowser-build into adblockbrowser fully, and then we would have a pretty clean situation. My only concern here is that we (René and I) discussed how to approach this when we first set up adblockbrowser-build and decided to put it in a separate repo. I wonder what our reasons were :D Do you remember something, René? The only thing that comes to mind is that Mozilla keeps the build logic externally (mozharness, Buildbot configs etc.), but if that's the only reason we'd be cargo culting.

If we can't come up with any reasons, I can change the description of this issue so we either integrate adblockbrowser-build into adblockbrowser or at least include it as a dependency, triggering the build from a script in adblockbrowser like Vasily suggested.

Last edited 4 years ago by fhd (previous) (diff)

comment:8 Changed 3 years ago by fhd

  • Cc diegocarloslima added
  • Description modified (diff)

Adding Diego, and picking this up again.

OK, so we want to move adblockbrowser-build into the adblockbrowser repository, makes most sense that way, and requires almost no code changes. I've updated the issue.

René/Vasily, please have another look at the updated description.

comment:9 Changed 2 years ago by fhd

  • Owner changed from fhd to rjeschke

I'll stop hugging this one :P

René wants to take it on, so I reassigned it to him.

comment:10 Changed 2 years ago by rjeschke

  • Description modified (diff)
  • Ready set

comment:11 Changed 2 years ago by rjeschke

  • Description modified (diff)

comment:12 Changed 2 years ago by rjeschke

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

comment:13 Changed 2 years ago by rjeschke

  • Description modified (diff)

comment:14 Changed 2 years ago by rjeschke

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:15 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 4105 - [build Make it possible to build arbitrary ABB revisions]

comment:16 Changed 2 years ago by diegocarloslima

  • Milestone set to Adblock-Browser-for-Android-1.3.3
  • Platform changed from Unknown / Cross platform to Adblock Browser for Android

comment:17 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 4105 - Mark adblockbrowser-build repository as deprecated

comment:18 Changed 2 years ago by fhd

I've filed http://hub.eyeo.com/issues/5864 (internal) for making the repository read-only and marking it as deprecated.

I don't think there's anything else we need to do :)

comment:19 Changed 2 years ago by diegocarloslima

  • Resolution fixed deleted
  • Status changed from closed to reopened

I'm currently having the following error when trying to run build.py in abb-build:

Client025:abb-build dlima$ ./build.py build
INFO: Cloning repository https://hg.adblockplus.org/mozharness into /Users/dlima/Workspace/adblockbrowser/abb-build/mozharness
Traceback (most recent call last):
  File "/Users/dlima/Workspace/adblockbrowser/abb-build/ensure_dependencies.py", line 380, in <module>
    resolve_deps(repo)
  File "/Users/dlima/Workspace/adblockbrowser/abb-build/ensure_dependencies.py", line 323, in resolve_deps
    ensure_repo(repodir, parenttype, target, vcs, _root.get(vcs, ''), source)
  File "/Users/dlima/Workspace/adblockbrowser/abb-build/ensure_dependencies.py", line 268, in ensure_repo
    repo_types[parenttype].ignore(target, parentrepo)
  File "/Users/dlima/Workspace/adblockbrowser/abb-build/ensure_dependencies.py", line 84, in ignore
    with open(config_path, 'w') as stream:
IOError: [Errno 2] No such file or directory: '/Users/dlima/Workspace/adblockbrowser/abb-build/.hg/hgrc'
Traceback (most recent call last):
  File "./build.py", line 160, in <module>
    subprocess.check_call([_ENSURE_DEPENDENCIES_PATH])
  File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 186, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['/Users/dlima/Workspace/adblockbrowser/abb-build/ensure_dependencies.py']' returned non-zero exit status 1

In the first attempt, it fails to run. Basically, this seems due to the fact that the ensure_dependencies.py is supposed to be in the root of the Mercurial repository and thus have a .hg folder inside of it. A possible solution would be to move the dependencies in abb-build folder to the root dependencies file and remove the ensure_dependencies.py inside abb-build

comment:20 Changed 2 years ago by diegocarloslima

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

comment:21 Changed 2 years ago by abpbot

A commit referencing this issue has landed:
Issue 4105 - [build Make it possible to build arbitrary ABB revisions]

comment:22 Changed 2 years ago by diegocarloslima

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