Support SIMPLE NAME for langpacks too

RESOLVED FIXED in Firefox 53

Status

RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: Callek, Assigned: Callek)

Tracking

unspecified
mozilla53

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
In order to implement "Chain Of Trust" for the taskcluster nightly graph, we need to keep all artifacts produced by the build system free of version information.

For beetmover and Balrog in taskcluster nightly, this also includes the en-US langpack xpi.

Taskcluster jobs are the only thing setting SIMPLE NAME, and SIMPLE NAMES are currently not set for l10n repack jobs (due to a set of bugs that make it currently impossible).

This patch makes SIMPLE NAME reflected for langpacks as well, and stops stuffing them in a sub-folder. So with an output like MOZ_SIMPLE_PACKAGE_NAME=target

The langpack will be named:

target.langpack.xpi alongside the target.tar.bz2 and so forth.

There is a try push of this code at https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0c2929da0bc24fade3c3d5793e74696f1c12f14  (this push was based off `date` project branch, to allow me to schedule linux nightlies as well. but applies cleanly to central)
Comment hidden (mozreview-request)

Comment 2

2 years ago
mozreview-review
Comment on attachment 8824532 [details]
Bug 1329310 - Support SIMPLE NAME for langpacks too.

https://reviewboard.mozilla.org/r/102994/#review103576

The two non-nits are more questions for you since I'm not super familiar with the langpacks. If we have some flexibility with how they're named and where they're located, I think this code could be simplified quite a bit.

::: toolkit/mozapps/installer/package-name.mk:76
(Diff revision 1)
>  COMPLETE_MAR = $(PKG_UPDATE_PATH)$(PKG_UPDATE_BASENAME).complete.mar
>  # PARTIAL_MAR needs to be processed by $(wildcard) before you use it.
>  PARTIAL_MAR = $(PKG_UPDATE_PATH)$(PKG_UPDATE_BASENAME).partial.*.mar
> +ifdef MOZ_SIMPLE_PACKAGE_NAME
> +PKG_LANGPACK_BASENAME = $(MOZ_SIMPLE_PACKAGE_NAME).langpack
> +PKG_LANGPACK_PATH = 

nit: trailing whitespace

::: toolkit/mozapps/installer/package-name.mk:78
(Diff revision 1)
>  PARTIAL_MAR = $(PKG_UPDATE_PATH)$(PKG_UPDATE_BASENAME).partial.*.mar
> +ifdef MOZ_SIMPLE_PACKAGE_NAME
> +PKG_LANGPACK_BASENAME = $(MOZ_SIMPLE_PACKAGE_NAME).langpack
> +PKG_LANGPACK_PATH = 
> +else
>  PKG_LANGPACK_BASENAME = $(MOZ_PKG_APPNAME)-$(MOZ_PKG_VERSION).$(AB_CD).langpack

Do you know if there's a reason why the PKG_LANGPACK_BASENAME is basically the same as PKG_BASENAME but without the $(MOZ_PKG_PLATFORM)? Specifically, I'm wondering if we can just do PKG_LANGPACK_BASENAME = $(PKG_BASENAME).langpack and remove the ifdef. It would change the filename for the non-simple package name case since it would now include $(MOZ_PKG_PLATFORM), but I'm not sure if that's relevant.

::: toolkit/mozapps/installer/package-name.mk:79
(Diff revision 1)
> +ifdef MOZ_SIMPLE_PACKAGE_NAME
> +PKG_LANGPACK_BASENAME = $(MOZ_SIMPLE_PACKAGE_NAME).langpack
> +PKG_LANGPACK_PATH = 
> +else
>  PKG_LANGPACK_BASENAME = $(MOZ_PKG_APPNAME)-$(MOZ_PKG_VERSION).$(AB_CD).langpack
>  PKG_LANGPACK_PATH = $(MOZ_PKG_PLATFORM)/xpi/

Any idea why we actually need a PKG_LANGPACK_PATH? It doesn't really seem to do much. I wonder if we can just delete it.
Attachment #8824532 - Flags: review?(mshal) → review+
Callek's patch needs some followup.  :jlund, if we're blocked:
a) you could land this patch on date directly
b) we could hardcode the version in the upstreamArtifacts paths until this patch lands and merges into date.
I think the above would unblock you.
Flags: needinfo?(jlund)
(Assignee)

Comment 4

2 years ago
(In reply to Michael Shal [:mshal] from comment #2)
> Comment on attachment 8824532 [details]
> Bug 1329310 - Support SIMPLE NAME for langpacks too.
> 
> https://reviewboard.mozilla.org/r/102994/#review103576
> 
> The two non-nits are more questions for you since I'm not super familiar
> with the langpacks. If we have some flexibility with how they're named and
> where they're located, I think this code could be simplified quite a bit.

I'm almost certain there is room for simplification. My goal here was to do the simplest and best change to support the tier1 requirement for tc-nightly. Given a team-self-imposed Deadline of code-complete Jan 10.

I'll likely also need to touch this code to support SIMPLE NAMES for l10n on repack tasks (multiple locales repacked in one job).

> ::: toolkit/mozapps/installer/package-name.mk:78
> (Diff revision 1)
> >  PARTIAL_MAR = $(PKG_UPDATE_PATH)$(PKG_UPDATE_BASENAME).partial.*.mar
> > +ifdef MOZ_SIMPLE_PACKAGE_NAME
> > +PKG_LANGPACK_BASENAME = $(MOZ_SIMPLE_PACKAGE_NAME).langpack
> > +PKG_LANGPACK_PATH = 
> > +else
> >  PKG_LANGPACK_BASENAME = $(MOZ_PKG_APPNAME)-$(MOZ_PKG_VERSION).$(AB_CD).langpack
> 
> Do you know if there's a reason why the PKG_LANGPACK_BASENAME is basically
> the same as PKG_BASENAME but without the $(MOZ_PKG_PLATFORM)? Specifically,
> I'm wondering if we can just do PKG_LANGPACK_BASENAME =
> $(PKG_BASENAME).langpack and remove the ifdef. It would change the filename
> for the non-simple package name case since it would now include
> $(MOZ_PKG_PLATFORM), but I'm not sure if that's relevant.

Offhand I'm not sure, but I do know doing this would affect production BB stuff. And I want to avoid a change there at the moment, until I get more time to vet these changes. Where they may be more risky.

We of course also have a difference here between "Pretty Names" and non-pretty. So the var is used:
https://dxr.mozilla.org/mozilla-central/source/toolkit/locales/l10n.mk#173
But is defined either where I patched or at https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/package-name.mk#115

Lastly the without MOZ_PKG_PLATFORM is because in the non-simple-format I'm doing it gets put in a path that is keyed off MOZ_PKG_PLATFORM by itself. (https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/package-name.mk#75 -- https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/package-name.mk#116 )

> ::: toolkit/mozapps/installer/package-name.mk:79
> (Diff revision 1)
> > +ifdef MOZ_SIMPLE_PACKAGE_NAME
> > +PKG_LANGPACK_BASENAME = $(MOZ_SIMPLE_PACKAGE_NAME).langpack
> > +PKG_LANGPACK_PATH = 
> > +else
> >  PKG_LANGPACK_BASENAME = $(MOZ_PKG_APPNAME)-$(MOZ_PKG_VERSION).$(AB_CD).langpack
> >  PKG_LANGPACK_PATH = $(MOZ_PKG_PLATFORM)/xpi/
> 
> Any idea why we actually need a PKG_LANGPACK_PATH? It doesn't really seem to
> do much. I wonder if we can just delete it.

Its used in the aforementioned langpack-% target's dep (for the .xpi itself) and then also with NSINSTALL to properly setup the folder. (https://dxr.mozilla.org/mozilla-central/source/toolkit/locales/l10n.mk#178)
Comment hidden (mozreview-request)

Comment 6

2 years ago
Pushed by Callek@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5fb6295f8fb8
Support SIMPLE NAME for langpacks too. r=mshal
(Assignee)

Comment 7

2 years ago
Since mike gave me r+ with those comments/questions I fixed up the whitespace nit and landed (via autoland) -- I also chose to land directly on date, since I won't be merging from central until monday and wanted to keep everyone unblocked until then.

https://hg.mozilla.org/projects/date/rev/f0737006e97caf488752eecafa8b35416caef821
(Assignee)

Updated

2 years ago
Flags: needinfo?(jlund)

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5fb6295f8fb8
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

6 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.