410922 - Allow uploading any extensions onto stage through tinder-config

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Tinderbox changes - v1

To allow uploading other extensions too (i.e gdata-provider), these changes to post-mozilla-rel.pl will allow specification of extensions to be copied to stage in a general manner.

To do so, adding the following var to tinder-config.pl is enough:

@extensions_upload = ('lightning.xpi', 'lightning-wcap.xpi', 'gdata-provider.xpi')


This will copy those files from $builddir/dist/(universal/)xpi-stage to $stagedir/<os>-xpi/. The universal bit is only used when $MacUniversalBinary is set. I hope the paths are correct, I have had no means of really testing this in a real-life scenario. The testing I have been able to do with --skip-mozilla does copy the files into ..../packages/windows-xpi/

I am open for input and it would be great if someone else could test this independant of me, since changes to post-mozilla-rel.pl can be very devastating :)

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Default tinderbox changes

Splitting up the patch into the mozilla/tools/tinderbox changes...

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: tinderbox-configs for lightning

... and the mozilla/tools/tinderbox-configs changes.

Comment 3

[Chris Cooper [:coop] (he/him)]

Comment on attachment 296013 [details] [diff] [review]
Default tinderbox changes

Looks good.

Comment 4

[Robert Kaiser]

Looks like this is trying to do the same I have been working on in bug 398711 - Philipp, does your patch cover anything my patch wouldn't have covered or the other way round?

Comment 5

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 296014 [details] [diff] [review]
tinderbox-configs for lightning

looks good. r=mvl

Comment 6

[Robert Kaiser]

Comment on attachment 296013 [details] [diff] [review]
Default tinderbox changes

>Index: tinderbox/tinder-defaults.pl
>===================================================================
>+
>+# Any extensions that are built can be uploaded to the stage server using these
>+# config options. These variables are mainly used by post-mozilla-rel.pl
>+#@extensions_upload = ('lightning.xpi', 'lightning-wcap.xpi', 'gdata-provider.xpi');
>+#$extensions_stage_subdir = "xpi"; # If not specified, <os>-xpi will be used.

I don't think those should be the defaults. The default should be to not upload any extension and if one is present, the default should be to use the platform subdirs.

Additionally, I dislike that one cannot upload any extension without creating a subdir for it...

Comment 7

[Robert Kaiser]

Comment on attachment 296014 [details] [diff] [review]
tinderbox-configs for lightning

>Index: tinderbox-configs/lightning/linux/tinder-config.pl
>===================================================================
>+
>+# Any extensions that are built can be uploaded to the stage server using these
>+# config options. These variables are mainly used by post-mozilla-rel.pl
>+@extensions_upload = ('lightning.xpi', 'lightning-wcap.xpi', 'gdata-provider.xpi');
>+#$extensions_stage_subdir = "xpi"; # If not specified, <os>-xpi will be used.

erm, I thought Lightning is platform-specific due to binary code and needs the platform subdirs?

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Default tinderbox changes

Slight change of variable name, idea from bug 398711. I find the new name fits better.

I also removed the change to tinder-defaults.pl, since these changes are commented out by default.

Will make sure these changes are OK on IRC before checking in.

Comment 9

[Chris Cooper [:coop] (he/him)]

Comment on attachment 296205 [details] [diff] [review]
Default tinderbox changes

Let's get a review from KaiRo first to make sure this aligns with what he needs in bug 398711. It would be nice to close both bugs in one go.

Comment 10

[Robert Kaiser]

Comment on attachment 296205 [details] [diff] [review]
Default tinderbox changes

>Index: tinder-config.pl
>===================================================================
>+#@ReleaseExtensions = ('lightning*.xpi', 'gdata-provider.xpi');
>+#$ReleaseExtensionSubdir = "xpi" # If not specified, <os>-xpi will be used.

I still would prefer to follow the style of the other settings and have the default values listed here as commented-out versions, and have those actually set as non-commented entries in the tinder-defaults file, as this is general style.

The real heart of the patch looks good for the moment, we can build upon that and improve it a bit more as a second step in bug 398711.

Based on that, r=me with the mentioned tinder-config/-defaults changes to follow common style of tinderbox code.

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Robert, since the default values in this case are empty, is it sufficient that they are not contained in tinder-defaults.pl, as it is in the latest patch? I did not include the tinder-defaults.pl there.

Comment 12

[Robert Kaiser]

The idea is that everybody can easily check that file and see what's used by default, and you'll see lots of empty default values actually in the current file, though I realized that some are breaking the convention and doing something similar to what you had in the first patch, but I consider those bugs (I'm not the maintainer of the tinderbox code though, just a contributor).

Comment 13

[Chris Cooper [:coop] (he/him)]

Comment on attachment 296205 [details] [diff] [review]
Default tinderbox changes

Do guys have the access you need to check this in? If not, reassign to me and I'll land it for you.

Comment 14

[Robert Kaiser]

I checked in both patches on behalf of Philipp and everything seems to still work! :)

Comment 15

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

the tinder-config.pl's for calendar were not checked in on branch, therefore we were missing branch xpi's since friday. I did the additional checkin, now everything should be fine.