Closed Bug 1301509 Opened 3 years ago Closed 3 years ago

Allow an override for l10n of ${PACKAGE} just for what EN_US has, without affecting resulting l10n packaging

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: Callek, Assigned: Callek)

Details

Attachments

(1 file)

So, in taskcluster we have SIMPLE_PACKAGE_NAME=target for all builds

At present defining that would not create localized binaries (when using chunking) well, since the package name targeted is always going to be the same...

In order to solve that, I am currently not defining the SIMPLE_PACKAGE_NAME for l10n, but in doing so, I'm unable to grab the `target` package since its looking for the non-simple name in its repacking.

This bug will solve that by allowing us to override the english package name, and thus be able to grab it from a task where the PACKAGE_NAME is different.

This was briefly discussed with mshal in IRC yesterday.

Better fix longer-term could be Bug 1300901 or something like Bug 1265537 but for more trees.
Comment on attachment 8789558 [details]
Bug 1301509 - Make EN_US_PACKAGE_NAME a thing, so we can avoid simple names in l10n.

https://reviewboard.mozilla.org/r/77732/#review76004

I thought we were discussing this idea as a way to help you test l10n without getting nightlies on Taskcluster. When the nightly builds are actually on Taskcluster, don't you want to get the 'target' package instead of 'firefox-51-whatever' package that buildbot uses?

::: toolkit/locales/l10n.mk:185
(Diff revision 1)
>  # This variable is to allow the wget-en-US target to know which ftp server to download from
>  ifndef EN_US_BINARY_URL 
>  EN_US_BINARY_URL = $(error You must set EN_US_BINARY_URL)
>  endif
>  
> +ifndef EN_US_PACKAGE_NAME

You can actually get rid of the ifndef and just use 'EN_US_PACKAGE_NAME ?= $(PACKAGE)' I believe.

::: toolkit/locales/l10n.mk:186
(Diff revision 1)
>  ifndef EN_US_BINARY_URL 
>  EN_US_BINARY_URL = $(error You must set EN_US_BINARY_URL)
>  endif
>  
> +ifndef EN_US_PACKAGE_NAME
> +EN_US_PACKAGE_NAME := $(PACKAGE)

This could use a comment to explain what it's used for.

::: toolkit/locales/l10n.mk:197
(Diff revision 1)
>  wget-en-US:
>  ifndef WGET
>  	$(error Wget not installed)
>  endif
>  	$(NSINSTALL) -D $(ABS_DIST)/$(PKG_PATH)
> -	(cd $(ABS_DIST)/$(PKG_PATH) && $(WGET) --no-cache -nv --no-iri -N  '$(EN_US_BINARY_URL)/$(PACKAGE)')
> +	(cd $(ABS_DIST)/$(PKG_PATH) && $(WGET) --no-cache \

nit: It might be a little easier to read if you split the line after the '&&', rather than in the middle of the wget command.

::: toolkit/locales/l10n.mk:198
(Diff revision 1)
>  ifndef WGET
>  	$(error Wget not installed)
>  endif
>  	$(NSINSTALL) -D $(ABS_DIST)/$(PKG_PATH)
> -	(cd $(ABS_DIST)/$(PKG_PATH) && $(WGET) --no-cache -nv --no-iri -N  '$(EN_US_BINARY_URL)/$(PACKAGE)')
> -	@echo 'Downloaded $(EN_US_BINARY_URL)/$(PACKAGE) to $(ABS_DIST)/$(PKG_PATH)/$(PACKAGE)'
> +	(cd $(ABS_DIST)/$(PKG_PATH) && $(WGET) --no-cache \
> +        -nv --no-iri -N  -O $(PACKAGE) '$(EN_US_BINARY_URL)/$(EN_US_PACKAGE_NAME)')

nit: double-space between '-N' and '-O'.
Attachment #8789558 - Flags: review?(mshal)
(In reply to Michael Shal [:mshal] from comment #3)
> Comment on attachment 8789558 [details]
> Bug 1301509 - Make EN_US_PACKAGE_NAME a thing, so we can avoid simple names
> in l10n.
> 
> https://reviewboard.mozilla.org/r/77732/#review76004
> 
> I thought we were discussing this idea as a way to help you test l10n
> without getting nightlies on Taskcluster. When the nightly builds are
> actually on Taskcluster, don't you want to get the 'target' package instead
> of 'firefox-51-whatever' package that buildbot uses?
> 

We were, my test yesterday was mainly a "can I support MOZ_SIMPLE_PACKAGE_NAME=target for l10n" -- to which I can't (right now).

I still hope to be able to switch to that at some point, but getting further on the "nightly graph" work is higher priority short-term than that build system fudging (given the complexity in the package name and l10n.mk stuff, I'd rather avoid it)

Fwiw the android build in the nightly graph is target.apk, but we can't use the SIMPLE name here because it chunks all l10n and then the second (and so on) repacks all get sent to the same package filename, and thus overwritten. So we can't do simple names for the output. The input however, can be whatever ;-) (doesn't need to match the same format, if we do this patch anyway)

...

I just submitted a new patch for this, with your nits addressed, and a new try push with locales trimmed (trimming on a not-to-be-committed-patch) so we can see the results without timeouts ;-)

If you need to chat about this outside of the bug, I am free most of the day tomorrow and can happily hop on vidyo.
(In reply to Justin Wood (:Callek) from comment #4)
> (In reply to Michael Shal [:mshal] from comment #3)
> > Comment on attachment 8789558 [details]
> > Bug 1301509 - Make EN_US_PACKAGE_NAME a thing, so we can avoid simple names
> > in l10n.
> > 
> > https://reviewboard.mozilla.org/r/77732/#review76004
> > 
> > I thought we were discussing this idea as a way to help you test l10n
> > without getting nightlies on Taskcluster. When the nightly builds are
> > actually on Taskcluster, don't you want to get the 'target' package instead
> > of 'firefox-51-whatever' package that buildbot uses?
> > 
> 
> We were, my test yesterday was mainly a "can I support
> MOZ_SIMPLE_PACKAGE_NAME=target for l10n" -- to which I can't (right now).
> 
> I still hope to be able to switch to that at some point, but getting further
> on the "nightly graph" work is higher priority short-term than that build
> system fudging (given the complexity in the package name and l10n.mk stuff,
> I'd rather avoid it)
> 
> Fwiw the android build in the nightly graph is target.apk, but we can't use
> the SIMPLE name here because it chunks all l10n and then the second (and so
> on) repacks all get sent to the same package filename, and thus overwritten.
> So we can't do simple names for the output. The input however, can be
> whatever ;-) (doesn't need to match the same format, if we do this patch
> anyway)

So with this patch, the android l10n in TC is dependent on android nightlies being in buildbot, right? If that's your short-term goal I think this is fine, though you're still going to need to support either TC nightlies not using a simple package name, or have l10n repackaging use a simple name + locale in order to get everything in TC.
Comment on attachment 8789558 [details]
Bug 1301509 - Make EN_US_PACKAGE_NAME a thing, so we can avoid simple names in l10n.

https://reviewboard.mozilla.org/r/77732/#review76288

::: toolkit/locales/l10n.mk:185
(Diff revision 2)
>  # This variable is to allow the wget-en-US target to know which ftp server to download from
>  ifndef EN_US_BINARY_URL 
>  EN_US_BINARY_URL = $(error You must set EN_US_BINARY_URL)
>  endif
>  
> +# Allow the overrideing of PACKAGE format so we can get an EN_US build with a different

nit: overriding
Attachment #8789558 - Flags: review?(mshal) → review+
11:04 AM <Callek> mshal: I'll put it in the bug -- but re: Bug 1301509 ----- not exactly
11:04 AM <mshal> ok :)
11:04 AM <Callek> mshal: the TC nightlies have a simple name, the TC l10n *cannot* use simple names (or we overwrite all the repacks) -- using simple names in the TC l10n is why we'd find target.apk, but has the aforementioned bustage....
11:05 AM <Callek> mshal: so this patch is to *support* passing target.apk as the binary to look for in TC l10n, while at the same time having `firefox....en-GB...apk` style package naming
11:05 AM <mshal> ahh, I had it flipped in my head
11:06 AM <Callek> I'll want to eventually support output as SIMPLE_NAMES as well, but that needs more handholding in build system and more care to get it right
11:06 AM <mshal> yeah
11:06 AM <Callek> mshal: given that does your r+ still stand (to be clear)
11:06 AM <mshal> yep
11:06 AM <mshal> thanks for explaining!
11:07 AM <Callek> no problem
Assignee: nobody → bugspam.Callek
Pushed by Callek@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/35c4ed28dbac
Make EN_US_PACKAGE_NAME a thing, so we can avoid simple names in l10n. r=mshal
https://hg.mozilla.org/mozilla-central/rev/35c4ed28dbac
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
given:

wget-en-US:
ifndef WGET
	$(error Wget not installed)
endif
	$(NSINSTALL) -D $(ABS_DIST)/$(PKG_PATH)
	(cd $(ABS_DIST)/$(PKG_PATH) && \
        $(WGET) --no-cache -nv --no-iri -N -O $(PACKAGE) '$(EN_US_BINARY_URL)/$(EN_US_PACKAGE_NAME)')
	@echo 'Downloaded $(EN_US_BINARY_URL)/$(EN_US_PACKAGE_NAME) to $(ABS_DIST)/$(PKG_PATH)/$(PACKAGE)'


if the system goes into $(ABS_DIST)/$(PKG_PATH), i.e. dist/win32/en-US,
and this is an empty directory, 

would "wget -O $(PACKAGE) ..." fail since it's trying to save the
ZIP file into win32/en-US/win32/en-US?  

looking at the logs, both Firefox and TB use a different way of doing
repacks (via downloadReleaseBuilds I think).

Is there some sort of environment variable magic that can not fail this
part?   Does Firefox/Thunderbird use this part of the code during
nightly?
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.