Closed Bug 1573570 Opened 3 years ago Closed 3 years ago

Include prebuilt OTR libraries in Thunderbird CI builds


(Thunderbird :: Build Config, enhancement)

Not set


(Not tracked)



(Reporter: rjl, Assigned: rjl)




(1 file, 4 obsolete files)

First round of getting OTR messaging into Thunderbird is going to involve downloading and caching prebuilt versions of libotr itself and its dependencies.

Taskcluster can fetch, verify, and cache a URL without too much work. Fetch tasks are all defined in the Mozilla checkout using the "reference_loader". A new loader is needed that is able to combine the generated jobs data for the past few days. This new "merge_loader" will make use of the current "reference_loader" implementation as well as "transform_loader".

The basic idea is to create a new loader that will make use of the existing reference_loader to look for kind definitions in M-* first, then use the "jobs-from" functionality to bring in some jobs defined in C-*.

Attached patch Patch 1 - merge_loader.patch (obsolete) — Splinter Review
This is the Taskcluster merge_loader.

Using the merge loader, tasks within a single kind can come from
both M-C and C-C trees. The implementation calls the reference_loader
function followed by the transform_loader function, merging the
two loaders into one. 

As noted in the commit message, there are issues when the kind
has dependencies. I believe it is only a problem when a task
within a kind depends on another task of the same kind. This
happens quite a bit with toolchains for instance.

The initial use case for this is for some fetch tasks, and there
are no dependency issues there.
Attachment #9085303 - Flags: review?(geoff)
Assignee: nobody → rob
Attached patch Patch 2 - otr_fetches.patch (obsolete) — Splinter Review
Task configuration that grabs Kai's prebuilt OTR libraries from his
server so they can be included in Taskcluster builds.

Uses merge_loader.

Currently broken.
Attachment #9085303 - Flags: review?(geoff)
Comment on attachment 9085304 [details] [diff] [review]
Patch 2 - otr_fetches.patch

Nit in commit message: This just sets up the Taskcluster pieec.
Attached patch Patch 2 - otr_fetches.patch (obsolete) — Splinter Review
Latest update of patch. This is working now.
Attachment #9085304 - Attachment is obsolete: true
Comment on attachment 9085303 [details] [diff] [review]
Patch 1 - merge_loader.patch

No changes have been made to this patch since initial submission. It's been working well.
Attachment #9085303 - Flags: review?(geoff)
Comment on attachment 9089913 [details] [diff] [review]
Patch 2 - otr_fetches.patch

This are the fetch tasks that will retrieve Kai's prebuilt libraries and modifications to the build configurations so they depend on those files.
Only submitting for review, waiting for a green light to actually land these.
Attachment #9089913 - Flags: review?(geoff)

I confirm the file hashes are correct.

Attached patch Patch 3 - otr_packaging.patch (obsolete) — Splinter Review
If you have suggestions to improve, let me know.
Hopefully some creative Makefile flags can improve the situation in the next
Attachment #9090112 - Flags: review?(geoff)
Summary: Create new Taskcluster loader to look for definitions in comm- and mozilla- in a single kind → Include prebuilt OTR libraries in Thunderbird CI builds
Typo fix.
Attachment #9090119 - Flags: review?(geoff)
Attachment #9090112 - Attachment is obsolete: true
Attachment #9090112 - Flags: review?(geoff)
Attachment #9085303 - Flags: review?(geoff) → review+
Attachment #9089913 - Flags: review?(geoff) → review+
Comment on attachment 9090119 [details] [diff] [review]
Patch 3 - otr_packaging.patch

Review of attachment 9090119 [details] [diff] [review]:

This seems fine.

Just for the record, this is only a review of the code. I'm not making a comment on whether or not it should be done this way (or at all) – those decisions are for other people.

::: third_party/
@@ +15,5 @@
> +	libgpg-error-0.dll \
> +	libotr-5.dll \
> +	libssp-0.dll
> +else
> +ifeq (Darwin,$(OS_ARCH))

I'm no expert on Makefiles, so ignore me if I'm wrong, but I think if this was "else ifeq …" you'd only need one endif.

@@ +24,5 @@
> +else
> +OTR_LIBS = \
> + \
> + \
> +

Attachment #9090119 - Flags: review?(geoff) → review+

Rob, as soon as this bug is fixed, will all published builds contain the binaries?

Can you also port this to the 68.x branch?

Depends on: 1592162
Attachment #9085303 - Attachment is obsolete: true
Attachment #9089913 - Attachment is obsolete: true

Bug 1518166 is ready pending a couple of reviews/approvals so there's no need to use the prebuilt libraries.

Closed: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.