Closed Bug 1370506 Opened 7 years ago Closed 7 years ago

Make installers-% "just work", hard-code build internals or provide sane defaults

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: Pike, Assigned: Pike)

References

(Blocks 2 open bugs)

Details

User Story

./mach installers-de should just work.

Hide l10n-merge, and hard-code the directory in which we do that in the build system.

Check out locales from l10n-central if needed. L10NBASEDIR should have a sound default, ~/.mozbuild/l10n-central.
The automatic check-out of l10n-repos is for Nightly only, to not affect beta builds, which require dedicated revisions. Also, I don't bother to update the l10n repos.

wget-en-US' EN_US_BINARYURL should have a sane default. Which I found for desktop. Not sure if there's one for mobile, and if I can deduce that from config.status.

Create a reliable make variable for when you're in an l10n repack task, and when you're not, and make that independent of configure. Also, have one for langpacks explicitly.


Workflows:

For developers:

  ./mach package
  ./mach build installers-de

For repacks:

  ./mach wget-en-US
  ./mach build installers-de

Attachments

(10 files, 4 obsolete files)

bug 1370506, don't use multiple object dirs in one mozharness run,
59 bytes, text/x-review-board-request
Callek
: review+
Details
bug 1370506, always merge for l10n repacks, with internally set merge dir,
59 bytes, text/x-review-board-request
glandium
: review+
Details
bug 1370506, for Nightly builds, automatically clone l10n repos for localized installers,
59 bytes, text/x-review-board-request
glandium
: review+
Details
bug 1370506, good default download URL for Desktop l10n repacks,
59 bytes, text/x-review-board-request
glandium
: review+
Details
bug 1370506, add l10n-related targets to top-level build.mk,
59 bytes, text/x-review-board-request
glandium
: review+
Details
bug 1370506, update the docs,
59 bytes, text/x-review-board-request
glandium
: review+
Details
bug 1370506, libs-% and chrome-% targets should set AB_CD,
59 bytes, text/x-review-board-request
glandium
: review+
Details
bug 1370506, removed un-used use of VPATH, bookmarks.inc doesn't exist anymore,
59 bytes, text/x-review-board-request
glandium
: review+
Details
bug 1370506, use MERGE_FILE instead of vpath to pick up merged files,
59 bytes, text/x-review-board-request
glandium
: review+
Details
new obj-l10n entries that we got since I last revised the patch
1.32 KB, patch
Callek
: review+
Details | Diff | Splinter Review 
As part of our move towards compare-locales 2.0, we changed what's an OK value for the merge dir.

And the ones we use right now are not OK ;-)

Can we change them to be such that they're a locale dir in a base dir? Like, PWD/merges/$(AB_CD) ?

The reason for this change is to get rid of assumptions in compare-locales, and the new scheme for merge dirs is that it looks like the localization. Which doesn't have to be a directory per locale anymore, which is why when the locale is a directory, the merged content is "passed-in-base/ab-CD".

Callek, can you help with that? This is going to block the cross-channel l10n work.
Blocks: 1372254
So, catlee told me last week he wasn't aware of enough information about what this blocks on the L10n team to make a good estimate-of-priority.  Can you help elaborate on that here or to him directly [for someone-in-releng to do it]?

That said, the requirements here are pretty much:

* Modify Buildbot (Includes Thunderbird) [affects all branches]
* Modify Taskcluster+Mozharness [in tree]

There is a fair amount of risk here (due to general riskiness of l10n codepaths as they exist today), especially because there is no good way to limit this per-branch, so if we were able to not require this change that would be best...

Some areas of code for this:

https://dxr.mozilla.org/build-central/search?q=merged+path%3Afactory.py&redirect=false

https://dxr.mozilla.org/build-central/source/tools/lib/python/build/misc.py#9

https://dxr.mozilla.org/build-central/search?q=path%3Al10n.py+merged&redirect=false  [not sure what if anything this file is used for offhand]

https://dxr.mozilla.org/mozilla-central/source/build/docs/locales.rst [docs only]

https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/l10n/locales.py#197

https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/l10n/multi_locale_build.py#159 [may not need a change]

=== (for pike's knowledge only) ===

Stuff in jar.py https://dxr.mozilla.org/mozilla-central/search?q=merged+path%3Ajar.py&redirect=false

installers-x-test: https://dxr.mozilla.org/mozilla-central/source/browser/locales/Makefile.in#200


It's also entirely possible I missed something, somewhere. By moving the mergedir we'd need testing on variants of this all, especially incomplete locales (and near-complete locales) to be sure merging works right.  Including testing on android with system locale set to non-english to verify those gecko strings too.
Flags: needinfo?(l10n)
As per IRC from yesterday, we have another way to get to the desired result, and that is to just ignore what the automation passes to the build.

For that, resummarizing and moving over to Core Build

I've got a WIP patch to do that, which I'd love to push to try, which makes me ask my general question...

Callek, how's l10n on try working right now?



To answer the real question that Callek asked:

Being able to mangle our directory structure is a big part of cross-channel localization, which is what makes l10n thrive post-dawn. We're not there yet, but we want to make it in 56, so both important and urgent.
Component: General Automation → Build Config
Flags: needinfo?(l10n) → needinfo?(bugspam.Callek)
Product: Release Engineering → Core
QA Contact: catlee
Summary: Change merge dir in automation to mergedir/$(AB_CD) → Hard-code merge dir in build logic to merge-dir/$(AB_CD)
Version: other → unspecified
L10n on try *should* work just fine right now. Ping me if any issues and I can assist in clearing up
Flags: needinfo?(bugspam.Callek)
Morphing this bug once more, and taking.

As I started looking at hiding the mergedir from automation, I realized that the tricks I do work for all the other oddities in the l10n repack system. I went for "let me do that for you", with --with-l10n-base-dir being the only developer preference left. And that one has a default now, so if you don't pick one, you get one.

I'll hit this with patches, and also send out the PI request.

Risk: not much. Actually, probably removing risk, in particular around automation changes, as I limit what automation can do, and thus what it can break.
Also, all the things we change here are executed as part of the l10n builds on try, so we get good feedback from try runs (yep, thanks Callek, they just work)

Reward: ./mach installers-de just works.

I decided to unconditionally run l10n-merge as part of the installers-% and langpack-% tasks. That will lead to slightly longer build times in automation. I think that's OK, because this way around, it's soooo much easier to fix the automation as you just need to make changes at one spot at a time. Basically, once this lands, automation can just remove code. And do so only in TC, or only in buildbot, and not care.

With these patches, artifact builds and l10n builds also shouldn't differ in principle. That opens up opportunities to rip out code, too.

The patches I have should also allow us to modify the jar.mn's and make them platform-independent again for language packs.
Assignee: nobody → l10n
User Story: (updated)
Summary: Hard-code merge dir in build logic to merge-dir/$(AB_CD) → Make installers-% "just work", hard-code build internals or provide sane defaults
This went to try in mostly the same way as https://treeherder.mozilla.org/#/jobs?repo=try&revision=dee85f40d631dde93c1ed6d071d1bdfd4a0ec385, with reduced locale set, and with the last bustage fix commit.
Comment on attachment 8878479 [details]
bug 1370506, don't take the l10n merge dir from the environment,

https://reviewboard.mozilla.org/r/149834/#review154516

I started playing around with comm-central, and realized MOZ_BUILD_APP doesn't work, as MOZ_BUILD_APP is '../mail' or even '../comm-central/mail' for me, that has a symlink from mozilla-central to comm-central/mozilla.

Is there a canonical way to get the app obj dir?

::: toolkit/locales/l10n.mk:57
(Diff revision 1)
>  	-DLOCALE_SRCDIR=$(abspath $(LOCALE_SRCDIR)) \
>  	-DPKG_BASENAME='$(PKG_BASENAME)' \
>  	-DPKG_INST_BASENAME='$(PKG_INST_BASENAME)' \
>  	$(NULL)
>  
> +BASE_MERGE:=$(topobjdir)/$(MOZ_BUILD_APP)/merge-dir

This would work over here, though I'm not sure if the calendar pieces of TB would like it:

    BASE_MERGE:=$(CURDIR)/merge-dir

::: tools/compare-locales/mach_commands.py:71
(Diff revision 1)
> -            try:
> +        try:
> -                # self.substs is raising an Exception if we're not configured
> +            # self.substs is raising an Exception if we're not configured
> -                # don't merge if we're not
> +            # don't merge if we're not
> -                merge_dir = mozpath.join(
> +            merge_dir = mozpath.join(
> -                    self.topobjdir,
> +                self.topobjdir,
> -                    self.substs['MOZ_BUILD_APP'],
> +                self.substs['MOZ_BUILD_APP'],

This never worked for comm-central, apparently.
(In reply to Axel Hecht [:Pike] from comment #15)
> Comment on attachment 8878479 [details]
> bug 1370506, don't take the l10n merge dir from the environment,
> 
> https://reviewboard.mozilla.org/r/149834/#review154516
> 
> I started playing around with comm-central, and realized MOZ_BUILD_APP
> doesn't work, as MOZ_BUILD_APP is '../mail' or even '../comm-central/mail'
> for me, that has a symlink from mozilla-central to comm-central/mozilla.
> 
> Is there a canonical way to get the app obj dir?

It really depends on where you are calling from (I know, it is a mess. Fixing it in bug 1366607). I think $(TOPOBJDIR)/$(MOZ_BUILD_APP) should get you to the right place in most cases, but it is not used often so I can't know. $(MOZ_BUILD_APP) is often just used in combination with $(TOPSRCDIR).


> This would work over here, though I'm not sure if the calendar pieces of TB
> would like it:
> 
>     BASE_MERGE:=$(CURDIR)/merge-dir
What are your concerns with the calendar pieces? Sounds reasonable to me.
(In reply to Philipp Kewisch [:Fallen] from comment #16)
> (In reply to Axel Hecht [:Pike] from comment #15)
> > Comment on attachment 8878479 [details]
> > This would work over here, though I'm not sure if the calendar pieces of TB
> > would like it:
> > 
> >     BASE_MERGE:=$(CURDIR)/merge-dir
> What are your concerns with the calendar pieces? Sounds reasonable to me.

Yeah, reading up on the mail/locales/makefile just now, there shouldn't be a problem. My concern was that there might be two Makefiles including l10n.mk from different places, and then the directory would be different.
Comment on attachment 8878483 [details]
bug 1370506, good default download URL for Desktop l10n repacks,

https://reviewboard.mozilla.org/r/149842/#review154980

::: browser/locales/Makefile.in:28
(Diff revision 1)
>  RETRIEVE_WINDOWS_INSTALLER = 1
>  
>  MOZ_LANGPACK_EID=langpack-$(AB_CD)@firefox.mozilla.org
> +# For Nightly, we know where to get the builds from to do local repacks
> +ifdef NIGHTLY_BUILD
> +EN_US_BINARY_URL=http://archive.mozilla.org/pub/firefox/nightly/latest-mozilla-central

Should use https, of course ;-)
Comment on attachment 8878484 [details]
bug 1370506, add l10n-related targets to top-level build.mk,

https://reviewboard.mozilla.org/r/149844/#review154976

::: Makefile.in:343
(Diff revision 1)
> +# These just redirect to MOZ_BUILD_APP/locales/Makefile.in,
> +# and are here for developer and automation convenience
> +ifndef TEST_MOZBUILD
> +ifdef MOZ_BUILD_APP
> +wget-en-US:
> +	$(MAKE) -C $(MOZ_BUILD_APP)/locales wget-en-US

Sadly, this doesn't seem to work for comm-central.
Comment on attachment 8878482 [details]
bug 1370506, make sure we run l10n-merge for installers-% and langpack-%,

https://reviewboard.mozilla.org/r/149840/#review154978

::: browser/locales/Makefile.in:142
(Diff revision 1)
>  # This is a generic target that will make a langpack, repack ZIP (+tarball)
>  # builds, and repack an installer if applicable. It is called from the
>  # tinderbox scripts. Alter it with caution.
>  
>  installers-%: IS_LANGUAGE_REPACK=1
> -installers-%: $(L10NSRCDIR) clobber-% langpack-% repackage-win32-installer-% repackage-zip-%
> +installers-%: $(L10NSRCDIR) clobber-% merge-% langpack-% repackage-win32-installer-% repackage-zip-%

The dependencies here are a bit weird, I'll fix that in a follow-up
(In reply to Axel Hecht [:Pike] from comment #19)
> Comment on attachment 8878484 [details]
> bug 1370506, add l10n-related targets to top-level Makefile,
> 
> https://reviewboard.mozilla.org/r/149844/#review154976
> 
> ::: Makefile.in:343
> (Diff revision 1)
> > +# These just redirect to MOZ_BUILD_APP/locales/Makefile.in,
> > +# and are here for developer and automation convenience
> > +ifndef TEST_MOZBUILD
> > +ifdef MOZ_BUILD_APP
> > +wget-en-US:
> > +	$(MAKE) -C $(MOZ_BUILD_APP)/locales wget-en-US
> 
> Sadly, this doesn't seem to work for comm-central.

Pushed a couple of fix-ups to my findings and pushed to try again.

That said, I run into a generic problem, and I wonder if you can help, glandium?

At a few places, I'm trying to do things in the object dir of the app, and I tried to use MOZ_BUILD_APP for that. Sadly, this doesn't do what I'm hoping for in comm-central, as it's just source based. With that, I tried to find a reliable way (in mach and also in the top-level Makefile.in) to get to the actual srcdir for comm-central, which I didn't find. Is there a way to get to OBJDIR/browser, mobile/android, mail, suite for Firefox, Fennec, TB, SM respectively?

Alternatively, the top-level l10n targets would all need to be copied to all of the */build.mk files?

And for the merge dir, we'd either need a global good place in the object dir to put it, or we let this bug take another slight turn:
As I'm forcefully merging to a internal location now, I could continue to pass the merge dir from my make rules to mach again. Maybe that's not such a bad idea?

Or, global place. Maybe OBJDIR/.mozbuild/l10n-merge/* ?

PS: If you'd prefer someone else to carry the review load on this one, feel free to forward.
Flags: needinfo?(mh+mozilla)
Comment on attachment 8878478 [details]
bug 1370506, don't use multiple object dirs in one mozharness run,

https://reviewboard.mozilla.org/r/149832/#review155866

::: commit-message-dced9:4
(Diff revision 1)
> +bug 1370506, don't use multiple object dirs in one mozharness run, r?Callek
> +
> +mozharness used to call compare-locales with obj-l10n as MOZ_OBJDIR, while
> +calling configure and installers-% with obj-firefox.

It's not clear to me where this was happening, can you provide (in bug is fine) a link/explanation.

Regardless of this happening or not, I don't object to these changes.
Attachment #8878478 - Flags: review?(bugspam.Callek) → review+
(In reply to Justin Wood (:Callek) from comment #32)
> It's not clear to me where this was happening, can you provide (in bug is
> fine) a link/explanation.

I didn't bother figuring out why, but it happens in single-locale android nightlies, at least.

https://public-artifacts.taskcluster.net/b3gZVx2MTX2DCjyadI-rlw/0/public/logs/live_backing.log is a log, and if you search for MOZ_OBJDIR, you'll see it switching back and forth as you go through the logs.
gps, glandium, can you get us next steps for this bug?

PS: I'm not going to be in SF next week, so while you guys might get some face-to-face time, I'll be best reached via this bug or email.
Flags: needinfo?(gps)
I still have to go through the whole thing, and I unfortunately won't be able to do so before next week. But I will, definitely look into it next week.
Comment on attachment 8878479 [details]
bug 1370506, don't take the l10n merge dir from the environment,

https://reviewboard.mozilla.org/r/149834/#review158472

::: toolkit/locales/l10n.mk:58
(Diff revision 2)
>  	-DPKG_BASENAME='$(PKG_BASENAME)' \
>  	-DPKG_INST_BASENAME='$(PKG_INST_BASENAME)' \
>  	$(NULL)
>  
> +BASE_MERGE:=$(CURDIR)/merge-dir
> +export REAL_LOCALE_MERGEDIR=$(BASE_MERGE)/$(AB_CD)

Is there a use case for make installers-AB_CD without LOCALE_MERGEDIR set at all? if not, why require that LOCALE_MERGEDIR=yes? It seems it would be simpler to just set LOCALE_MERGEDIR any time AB_CD is not the default locale?
Comment on attachment 8878479 [details]
bug 1370506, don't take the l10n merge dir from the environment,

https://reviewboard.mozilla.org/r/149834/#review158474
Attachment #8878479 - Flags: review?(mh+mozilla)
Comment on attachment 8878480 [details]
bug 1370506, always merge for l10n repacks, with internally set merge dir,

https://reviewboard.mozilla.org/r/149836/#review158476

I guess this answers the question from previous patch review... What is weird, though, is that you split that in two, while at the same time sneaking in another change in this patch: removing the vpaths. It would be less confusing if you separated that out.
Attachment #8878480 - Flags: review?(mh+mozilla)
Comment on attachment 8878480 [details]
bug 1370506, always merge for l10n repacks, with internally set merge dir,

https://reviewboard.mozilla.org/r/149836/#review158478

::: mobile/android/locales/Makefile.in:54
(Diff revision 2)
>  
>  # This is a generic target that will make a langpack and repack tarball
>  # builds. It is called from the tinderbox scripts. Alter it with caution.
>  
> -installers-%: clobber-stage repackage-zip-%
> +installers-%: IS_LANGUAGE_REPACK=1
> +installers-%: $(L10NSRCDIR) merge-% clobber-stage repackage-zip-%

you have a $(L10NSRCDIR) sneaking in here.
Comment on attachment 8878481 [details]
bug 1370506, for Nightly builds, automatically clone l10n repos for localized installers,

https://reviewboard.mozilla.org/r/149838/#review158480

::: mobile/android/locales/Makefile.in:54
(Diff revision 2)
>  
>  # This is a generic target that will make a langpack and repack tarball
>  # builds. It is called from the tinderbox scripts. Alter it with caution.
>  
>  installers-%: IS_LANGUAGE_REPACK=1
> -installers-%: $(L10NSRCDIR) merge-% clobber-stage repackage-zip-%
> +installers-%: $(L10NSRCDIR) clobber-stage repackage-zip-%

You added merge-% in previous patch, seems like you should just no add it in the first place.

::: toolkit/locales/l10n.mk:75
(Diff revision 2)
> +L10NSRCDIR = $(if $(filter $(AB_CD), en-US), ., $(L10NBASEDIR)/%)
> +ifdef NIGHTLY_BUILD
> +.PRECIOUS: $(L10NBASEDIR)/%
> +$(L10NBASEDIR)/%:
> +	$(NSINSTALL) -D $(L10NBASEDIR)
> +	hg --cwd $(L10NBASEDIR) clone https://hg.mozilla.org/l10n-central/$*/

hardcoding a VCS is not entirely nice.

Also, shouldn't this be refreshed if the directory already exists instead of leaving old clones around?

::: toolkit/moz.configure:109
(Diff revision 2)
>      if value:
>          path = value[0]
>          if not isdir(path):
>              die("Invalid value --with-l10n-base, %s doesn't exist", path)
> +    else:
> +        path = os.path.join(expanduser('~'), '.mozbuild', 'l10n-central')

os.path.join(os.environ.get('MOZBUILD_STATE_PATH', os.path.expanduser(os.path.join('~', '.mozbuild'))), 'l10n-central')
Attachment #8878481 - Flags: review?(mh+mozilla)
Comment on attachment 8878482 [details]
bug 1370506, make sure we run l10n-merge for installers-% and langpack-%,

https://reviewboard.mozilla.org/r/149840/#review158648
Attachment #8878482 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8878483 [details]
bug 1370506, good default download URL for Desktop l10n repacks,

https://reviewboard.mozilla.org/r/149842/#review158652

Not a fan, but this is not worse than what we currently do. Eventually, we should make repacks more "normal" artifacts builds.
Attachment #8878483 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8878481 [details]
bug 1370506, for Nightly builds, automatically clone l10n repos for localized installers,

https://reviewboard.mozilla.org/r/149838/#review158654

::: toolkit/locales/l10n.mk:75
(Diff revision 2)
> +L10NSRCDIR = $(if $(filter $(AB_CD), en-US), ., $(L10NBASEDIR)/%)
> +ifdef NIGHTLY_BUILD
> +.PRECIOUS: $(L10NBASEDIR)/%
> +$(L10NBASEDIR)/%:
> +	$(NSINSTALL) -D $(L10NBASEDIR)
> +	hg --cwd $(L10NBASEDIR) clone https://hg.mozilla.org/l10n-central/$*/

I don't think there's any intent to mirror this to other VCSs, nor that I expect that we'll ask people to edit it.

In many ways this is like v-c-t, you just get it, and if you need an update, you'll need to intentionally get it.

I pondered doing a pull on the l10n repo, but there's two reasons that I have against doing so by default:

- it's adding quite a bit of wall-clock time, at least in comparison to what an l10n-merge takes
- more importantly, if you're working on a build issue, and you get an idea of the expected output, it'd be very weird to have that change, just because the upstream localization is suddenly 100% complete or so.

I wonder if there's a good way to detect that both you haven't pulled in a while and that there's new content remotely.
Comment on attachment 8878484 [details]
bug 1370506, add l10n-related targets to top-level build.mk,

https://reviewboard.mozilla.org/r/149844/#review158658

::: Makefile.in:342
(Diff revision 2)
> +# Top-level l10n-related entry points.
> +# These just redirect to MOZ_BUILD_APP/locales/Makefile.in,
> +# and are here for developer and automation convenience
> +ifndef TEST_MOZBUILD
> +ifdef MOZ_BUILD_APP
> +wget-en-US:

This target doesn't really care about -jn... just fold it with the others below?

::: Makefile.in:347
(Diff revision 2)
> +wget-en-US:
> +	$(MAKE) -C $(MOZ_BUILD_APP)/locales wget-en-US
> +
> +# make -j1 because dependencies in l10n build targets don't work
> +# with parallel builds
> +merge-% installers-% langpack-% chrome-%:

how about defining all this in $MOZ_BUILD_APP/build.mk, along l10n-check?
Attachment #8878484 - Flags: review?(mh+mozilla)
Comment on attachment 8878486 [details]
bug 1370506, factor merge-% into l10n.mk, pass --l10n-base,

https://reviewboard.mozilla.org/r/149848/#review158662

::: commit-message-9a5a0:3
(Diff revision 2)
> +This is a bustage fix for Fennec Nightlies, which don't set the base
> +dir in the mozconfig, but take it from the environment.
> +With this fix, it works both ways.

It would be better to apply this as a preparatory patch rather than a fixup (as in patch 1 or 2 of 10, rather than 9 of 10(
Attachment #8878486 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8879195 [details]
bug 1370506, libs-% and chrome-% targets should set AB_CD,

https://reviewboard.mozilla.org/r/150532/#review158666

::: mobile/android/locales/Makefile.in:43
(Diff revision 1)
>  	@$(MAKE) -C $(DEPTH)/mobile/android/base/locales AB_CD=$* XPI_NAME=locale-$*
>  endif
>  
>  # Tailored target to just add the chrome processing for multi-locale builds
>  chrome-%: IS_LANGUAGE_REPACK=1
> +chrome-%: AB_CD=$*

It seems like the problem is that AB_CD is not explicitly passed to each MAKE command below. If you're going to set it this way, you should remove it in the one command that sets it, and also do the same change in all the */locales/Makefile.in places where something like this is done.
Attachment #8879195 - Flags: review?(mh+mozilla)
Comment on attachment 8878484 [details]
bug 1370506, add l10n-related targets to top-level build.mk,

https://reviewboard.mozilla.org/r/149844/#review158658

> This target doesn't really care about -jn... just fold it with the others below?

I had that originally, but make didn't like mixing pattern and non-pattern rules, which is why it's separate. The non-parallel stuff wasn't the reason for this being extra from the ones below.
Comment on attachment 8878479 [details]
bug 1370506, don't take the l10n merge dir from the environment,

https://reviewboard.mozilla.org/r/149834/#review158738

::: toolkit/locales/l10n.mk:58
(Diff revision 2)
>  	-DPKG_BASENAME='$(PKG_BASENAME)' \
>  	-DPKG_INST_BASENAME='$(PKG_INST_BASENAME)' \
>  	$(NULL)
>  
> +BASE_MERGE:=$(CURDIR)/merge-dir
> +export REAL_LOCALE_MERGEDIR=$(BASE_MERGE)/$(AB_CD)

I had to switch the makefiles away from using LOCALE_MERGEDIR as a variable, because you can't make make overwrite something that's coming from the environment. At least I haven't found a way to do it.

And yes, I remove the buildpaths without merge later in the patch queue.
(In reply to Axel Hecht [:Pike] from comment #48)
> I had to switch the makefiles away from using LOCALE_MERGEDIR as a variable,
> because you can't make make overwrite something that's coming from the
> environment. At least I haven't found a way to do it.

But why pass LOCALE_MERGEDIR at all on the command line in the first place was my point.

(note, you can't overwrite VAR in `make VAR=value`, but you can overwrite it in `VAR=value make`)
Flags: needinfo?(mh+mozilla)
Comment on attachment 8878485 [details]
bug 1370506, update the docs,

https://reviewboard.mozilla.org/r/149846/#review158794

::: build/docs/locales.rst:44
(Diff revision 2)
> +The ``installers`` target runs quite a few things for you, including getting
> +the repository for the requested locale from
> +https://hg.mozilla.org/l10n-central/. It will clone them into
> +``~/.mozbuild/l10n-central``. If you have an existing repository there, you
> +may want to occasionally update that via ``hg pull -u``. If you prefer
> +to have the l10n repositories at a different localition on your disk, you

typo: location
Attachment #8878485 - Flags: review?(mh+mozilla) → review+
I think glandium addressed the needinfo.
Flags: needinfo?(gps)
(In reply to Mike Hommey [:glandium] from comment #49)
> (In reply to Axel Hecht [:Pike] from comment #48)
> > I had to switch the makefiles away from using LOCALE_MERGEDIR as a variable,
> > because you can't make make overwrite something that's coming from the
> > environment. At least I haven't found a way to do it.
> 
> But why pass LOCALE_MERGEDIR at all on the command line in the first place
> was my point.

"Just fixing LOCALE_MERGEDIR" was my initial intent of this bug, but then Callek returned the list of locations where we need to do that, and which of those don't ride the train in comment 1. It's close to impossible to do that while we have buildbot around. Doing the merge independently, assuming no control over the call site, seems the smoothest sword cut through that gordian knot.

And then I figured I could just make the build system not care what it's passing.

And then I figured how about I just look at what happens if I enforce a merge. And so forth.

Which is also why the patch queue is taking turns.

I'll have it take one more turn, and then rebuild the patch queue: I will actually take the merge dir from the commandline argument in the mach command, but I'll ignore the one that the automation does, and rely on the one I automatically trigger as part of the repack.

Going through the feedback items now.
Comment on attachment 8878486 [details]
bug 1370506, factor merge-% into l10n.mk, pass --l10n-base,

https://reviewboard.mozilla.org/r/149848/#review158662

> It would be better to apply this as a preparatory patch rather than a fixup (as in patch 1 or 2 of 10, rather than 9 of 10(

I'll be folding this into the first real patch in the next iteration.
Comment on attachment 8879195 [details]
bug 1370506, libs-% and chrome-% targets should set AB_CD,

https://reviewboard.mozilla.org/r/150532/#review162018

::: mobile/android/locales/Makefile.in:43
(Diff revision 1)
>  	@$(MAKE) -C $(DEPTH)/mobile/android/base/locales AB_CD=$* XPI_NAME=locale-$*
>  endif
>  
>  # Tailored target to just add the chrome processing for multi-locale builds
>  chrome-%: IS_LANGUAGE_REPACK=1
> +chrome-%: AB_CD=$*

I'm going to shift this up in the patch queue, and make it consistent.

libs-% and chrome-% will get it, installers-% not, as that needs en-US for the unpackaging step. l10n.mk gets a comment for that.
Attachment #8878479 - Attachment is obsolete: true
Attachment #8878482 - Attachment is obsolete: true
Attachment #8878486 - Attachment is obsolete: true
Thanks for bearing with me, and sorry for the admittently confusing first patch queue. All I can say is that it adequatly reflected my confused brain, and that I needed some feedback to straighten it up.

I do think this queue is a lot nicer, so that's good, I hope.

Changes, at a high level: I reordered the AB_CD patch to come just after the mozconfigs, and then squashed most of the repack stuff in just one patch that works on its own. The follow-ups are really only for the the l10n repos, the top-level helpers, the EN_US_BINARY_URL, and docs.

The other change that's in here is applying the same changes to the browser chrome-% rule that gandalf landed in the meantime, as part of the relevant patches.
Comment on attachment 8878483 [details]
bug 1370506, good default download URL for Desktop l10n repacks,

https://reviewboard.mozilla.org/r/149842/#review162092

::: browser/locales/Makefile.in:28
(Diff revision 3)
>  RETRIEVE_WINDOWS_INSTALLER = 1
>  
>  MOZ_LANGPACK_EID=langpack-$(AB_CD)@firefox.mozilla.org
> +# For Nightly, we know where to get the builds from to do local repacks
> +ifdef NIGHTLY_BUILD
> +EN_US_BINARY_URL=https://archive.mozilla.org/pub/firefox/nightly/latest-mozilla-central

can we do ?= here, otherwise this will [I think] pretty likely break TC's passing of that in.
Comment on attachment 8878481 [details]
bug 1370506, for Nightly builds, automatically clone l10n repos for localized installers,

https://reviewboard.mozilla.org/r/149838/#review162448

::: toolkit/locales/l10n.mk:76
(Diff revision 3)
> +# For nightly builds, we automatically check out missing localizations
> +# from l10n-central.
> +L10NSRCDIR = $(if $(filter $(AB_CD), en-US), ., $(L10NBASEDIR)/%)
> +ifdef NIGHTLY_BUILD
> +.PRECIOUS: $(L10NBASEDIR)/%
> +$(L10NBASEDIR)/%:

In testing comm-central, I figured out that this worked in m-c practice, but not in theory.

Whenever a make rule depends on a l10n file, this line tries to clone that file as a repo. Let's not do this.

Fix is nicer, as the merge-% target is the only trigger of this, I'll fold this into the merge-% target allright.
Comment on attachment 8878483 [details]
bug 1370506, good default download URL for Desktop l10n repacks,

https://reviewboard.mozilla.org/r/149842/#review162092

> can we do ?= here, otherwise this will [I think] pretty likely break TC's passing of that in.

I did that, and I made this an export, too. Firefox doesn't need that, but thunderbird needs to pass that down to the makefile that tries to get lightning, it seems, and it doesn't hurt.
No longer blocks: 1372254
See Also: → 1372254
Blocks: 1382005
Blocks: 1382632
Comment on attachment 8878480 [details]
bug 1370506, always merge for l10n repacks, with internally set merge dir,

https://reviewboard.mozilla.org/r/149836/#review165338

::: commit-message-1e3fe:29
(Diff revision 3)
> +MozReview-Commit-ID: 3nr33CKxkBQ
> +MozReview-Commit-ID: EmeKnNh5Hzi
> +MozReview-Commit-ID: 9c5fhV4dfpu
> +MozReview-Commit-ID: 2erzhowoKZm

huh?

::: browser/locales/Makefile.in
(Diff revision 3)
> -ifdef LOCALE_MERGEDIR
> -vpath crashreporter%.ini $(LOCALE_MERGEDIR)/browser/crashreporter
> -endif
> -vpath crashreporter%.ini $(LOCALE_SRCDIR)/crashreporter
> -ifdef LOCALE_MERGEDIR
> -vpath crashreporter%.ini @srcdir@/en-US/crashreporter
> -endif

It feels like the crashreporter.ini changes are independent and should be in their own separate patch.

::: mobile/locales/Makefile.in
(Diff revision 3)
> -ifdef LOCALE_MERGEDIR
> -vpath book%.inc $(LOCALE_MERGEDIR)/mobile/profile
> -endif
> -vpath book%.inc $(LOCALE_SRCDIR)/profile
> -ifdef LOCALE_MERGEDIR
> -vpath book%.inc @srcdir@/en-US/profile
> -endif

Same as crashreporter.ini, this should have its own separate patch indicating it's useless because bookmarks.inc was removed in bug 1197054.
Attachment #8878480 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8879195 [details]
bug 1370506, libs-% and chrome-% targets should set AB_CD,

https://reviewboard.mozilla.org/r/150532/#review165342
Attachment #8879195 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8878481 [details]
bug 1370506, for Nightly builds, automatically clone l10n repos for localized installers,

https://reviewboard.mozilla.org/r/149838/#review165346

::: toolkit/locales/l10n.mk:188
(Diff revision 4)
> +# For nightly builds, we automatically check out missing localizations
> +# from l10n-central.
> +ifdef NIGHTLY_BUILD
> +	@if  ! test -d $(L10NBASEDIR)/$(AB_CD) ; then \
> +		$(NSINSTALL) -D $(L10NBASEDIR) ; \
> +		hg --cwd $(L10NBASEDIR) clone https://hg.mozilla.org/l10n-central/$(AB_CD)/ ; \

We now have VCS_CHECKOUT_TYPE, HG and GIT. Please do something like:

ifeq ($(VCS_CHECKOUT_TYPE),hg)
  $(HG) ... clone https://...
else
ifeq ($(VCS_CHECKOUT_TYPE),git)
  $(GIT) ... clone hg::https://...
endif
endif
Attachment #8878481 - Flags: review?(mh+mozilla)
Comment on attachment 8878484 [details]
bug 1370506, add l10n-related targets to top-level build.mk,

https://reviewboard.mozilla.org/r/149844/#review165350
Attachment #8878484 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8878480 [details]
bug 1370506, always merge for l10n repacks, with internally set merge dir,

https://reviewboard.mozilla.org/r/149836/#review165338

> huh?

I squashed commits, and didn't find docs on what to do with those. I removed all but the first, and it seems that mozreview is fine :-).
Comment on attachment 8878481 [details]
bug 1370506, for Nightly builds, automatically clone l10n repos for localized installers,

https://reviewboard.mozilla.org/r/149838/#review165346

> We now have VCS_CHECKOUT_TYPE, HG and GIT. Please do something like:
> 
> ifeq ($(VCS_CHECKOUT_TYPE),hg)
>   $(HG) ... clone https://...
> else
> ifeq ($(VCS_CHECKOUT_TYPE),git)
>   $(GIT) ... clone hg::https://...
> endif
> endif

Ah, now I get it, using git in a cinnabar way. Not trying to have upstream git repos.

I checked this to the extent that I could without setting up a full cinnabar-based environment, both that at least one of the l10n-central repos doesn't trigger anything fancy in git, and that the L10N_CO variable produces the expected result when making make think it's git.

Also rebased the whole queue to the current m-c state, so that it can get the VCS_CHECKOUT_TYPE.
Blocks: 1385227
This fails running locally when doing `./mach package` when using `--build-backends=RecursiveMake`. It doesn't error out when using the FasterMake backend due to Bug 1255096 (see https://dxr.mozilla.org/mozilla-central/source/browser/installer/Makefile.in#154 ).
Comment on attachment 8878480 [details]
bug 1370506, always merge for l10n repacks, with internally set merge dir,

https://reviewboard.mozilla.org/r/149836/#review168730

::: browser/locales/Makefile.in:89
(Diff revision 4)
>  	$(NSINSTALL) -D $@
>  
>  DEFINES += -DBOOKMARKS_INCLUDE_DIR=$(dir $(call MERGE_FILE,profile/bookmarks.inc))
>  
>  libs-%: AB_CD=$*
> -libs-%:
> +libs-%: merge-%

tomprince found that this breaks en-US langpacks.

Got a fix coming up for this.
Fixed the issue that Tom found, and kicked of a new set of try builds, with triggered nightlies, https://treeherder.mozilla.org/#/jobs?repo=try&revision=077fe67c66a9e72ce8abbfa6ae87d7dd897e41dc.

The $(if) code is weirder than I'd like it to be, but the straight conditionals in makefiles are evaluated early while AB_CD is still en-US regardless.
try is as green as it gets, filed a couple of bugs on try not working for l10n builds, starred the builds with those.

They're unrelated to the work here, and probably just indicate open items of getting osx and windows on taskcluster. Downloading target.zip instead of nightly, routes being funky on tc, things like that.
Comment on attachment 8889337 [details]
bug 1370506, removed un-used use of VPATH, bookmarks.inc doesn't exist anymore,

https://reviewboard.mozilla.org/r/160412/#review169466
Attachment #8889337 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8889338 [details]
bug 1370506, use MERGE_FILE instead of vpath to pick up merged files,

https://reviewboard.mozilla.org/r/160414/#review169468
Attachment #8889338 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8878481 [details]
bug 1370506, for Nightly builds, automatically clone l10n repos for localized installers,

https://reviewboard.mozilla.org/r/149838/#review169470
Attachment #8878481 - Flags: review?(mh+mozilla) → review+
Pushed by axel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4948080c1b7e
don't use multiple object dirs in one mozharness run, r=Callek
https://hg.mozilla.org/integration/autoland/rev/1d3ca513812f
libs-% and chrome-% targets should set AB_CD, r=glandium
https://hg.mozilla.org/integration/autoland/rev/04651043c479
removed un-used use of VPATH, bookmarks.inc doesn't exist anymore, r=glandium
https://hg.mozilla.org/integration/autoland/rev/1d93de40fff4
use MERGE_FILE instead of vpath to pick up merged files, r=glandium
https://hg.mozilla.org/integration/autoland/rev/8e37fd6588f2
always merge for l10n repacks, with internally set merge dir, r=glandium
https://hg.mozilla.org/integration/autoland/rev/55cfa8df80a4
for Nightly builds, automatically clone l10n repos for localized installers, r=glandium
https://hg.mozilla.org/integration/autoland/rev/64a69b2cebbb
good default download URL for Desktop l10n repacks, r=glandium
https://hg.mozilla.org/integration/autoland/rev/0ef6d21da190
add l10n-related targets to top-level build.mk, r=glandium
https://hg.mozilla.org/integration/autoland/rev/0fdb6a37c65c
update the docs, r=glandium
Blocks: 1386970
Attachment #8893369 - Attachment is patch: true
Attachment #8893369 - Flags: review+
Pushed by axel@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/fa1da3c0b200
fix remaining mozharness configs MOZ_OBJDIR, r=Callek, a=bustage
Blocks: 1388026
https://hg.mozilla.org/projects/date/rev/4948080c1b7e5fa7de4c74078b3dd310cd01181b
bug 1370506, don't use multiple object dirs in one mozharness run, r=Callek

https://hg.mozilla.org/projects/date/rev/1d3ca513812f4f4d924feb67a1f46650dd39e01a
bug 1370506, libs-% and chrome-% targets should set AB_CD, r=glandium

https://hg.mozilla.org/projects/date/rev/04651043c479d097d3cfc8ff91c0f25db2727892
bug 1370506, removed un-used use of VPATH, bookmarks.inc doesn't exist anymore, r=glandium

https://hg.mozilla.org/projects/date/rev/1d93de40fff4f093642b5fba3589d8b66e561975
bug 1370506, use MERGE_FILE instead of vpath to pick up merged files, r=glandium

https://hg.mozilla.org/projects/date/rev/8e37fd6588f22193c79ca9a1c7b1149d03a06c76
bug 1370506, always merge for l10n repacks, with internally set merge dir, r=glandium

https://hg.mozilla.org/projects/date/rev/55cfa8df80a4ce36ca96e342bc379b7dcead3111
bug 1370506, for Nightly builds, automatically clone l10n repos for localized installers, r=glandium

https://hg.mozilla.org/projects/date/rev/64a69b2cebbbbdb4f40133eff0f1711756d89a66
bug 1370506, good default download URL for Desktop l10n repacks, r=glandium

https://hg.mozilla.org/projects/date/rev/0ef6d21da19021b9547de282145b2ef5d85d3851
bug 1370506, add l10n-related targets to top-level build.mk, r=glandium

https://hg.mozilla.org/projects/date/rev/0fdb6a37c65caaffbcf04834589c13bda0b96cd4
bug 1370506, update the docs, r=glandium

https://hg.mozilla.org/projects/date/rev/fa1da3c0b200abbd9cfab3cab19962824314044e
bug 1370506: fix remaining mozharness configs MOZ_OBJDIR, r=Callek, a=bustage
Blocks: 1390461
Flags: qe-verify-
Pushed by mozilla@hocat.ca:
https://hg.mozilla.org/comm-central/rev/902f0dde1fa8
Port win32 locale/packaging changes to comm-central ; r=me
Attachment #8941701 - Attachment is obsolete: true
Product: Core → Firefox Build System
Depends on: 1836189
No longer depends on: 1836189
See Also: → 1836189
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: