Closed Bug 1409721 Opened 8 years ago Closed 8 years ago

Express localized files in moz.build

Categories

(Firefox Build System :: General, enhancement)

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

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: ted, Assigned: ted)

References

(Blocks 5 open bugs)

Details

Attachments

(4 files)

bug 1409721 - move crashreporter.ini and crashreporter-override.ini to moz.build LOCALIZED_FILES.
59 bytes, text/x-review-board-request
nalexander
: review+
Details
bug 1409721 - add LOCALIZED_FILES and LOCALIZED_PP_FILES to moz.build sandbox.
59 bytes, text/x-review-board-request
nalexander
: review+
Details
bug 1409721 - wire up support for LOCALIZED[_PP]_FILES in the recursive make backend.
59 bytes, text/x-review-board-request
nalexander
: review+
Details
bug 1409721 - move firefox-l10n.js to moz.build.
59 bytes, text/x-review-board-request
nalexander
: review+
Details
One of the biggest blockers to using a non-make build backend left are all the Makefile rules for localized files. We haven't been able to port those to moz.build because l10n repacks invoke make directly. The main source of pain is the `libs-%` target in browser/locales: https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/browser/locales/Makefile.in#92 which runs make in a bunch of directories, overriding $(AB_CD) to get them to copy localized files to the right places. In the short term, we'd like to be able to move the rules for copying files etc out of Makefile.in into moz.build so that we can do builds with an alternate build backend (like tup). My proposal is to add something like `LOCALIZED_FILES` to moz.build, which would act similar to `FINAL_TARGET_FILES`, but the files would come from the locale directory the way they do now. This should be sufficient for simple things like crashreporter.ini and crashreporter-override.ini: https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/toolkit/locales/Makefile.in#29 https://dxr.mozilla.org/mozilla-central /rev/20d57b9c4183973af4af5e078dff2aec0b74f928/browser/locales/Makefile.in#190 as well as hunspell dictionaries: https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/extensions/spellcheck/locales/Makefile.in#5 and the firefox-l10n.js pref file: https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/browser/locales/Makefile.in#31 We'll also need to make `GENERATED_FILES` (or something like it) work, to support things like update.locale work: https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/toolkit/locales/Makefile.in#24 and updater.ini: https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/browser/locales/Makefile.in#171 There are a couple of things that might be a little hard to shoehorn in here, like searchplugins, which gets the list of filenames to install at build time: https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/browser/locales/Makefile.in#61 If we add those things and wire them up to the recursive make backend to generate Makefile rules that work functionally the same as what we currently have, that should allow tup builds to work (by generating its own set of rules for these targets), and also keep l10n repacks working by continuing to provide the Makefile targets they rely on. Reworking l10n repacks completely is still something we're interested in, but we don't have to solve that right now.
FWIW, I instituted two approaches in mobile/android that helped with this nonsense: - run generation steps in every tier, using FORCE and a Python script with FileAvoidWrite to get around the different tiers and directories that repacks and builds needed things to run in. See http://searchfox.org/mozilla-central/source/mobile/android/base/Makefile.in#347-366 and the many interactions around it. - as I said on IRC, I set a small pattern for generating local-aware files around http://searchfox.org/mozilla-central/source/mobile/android/base/locales/Makefile.in#98-114 I'm excited to simplify a lot of this noise, it's holding back the Fennec team.
I recall previous attempts at moving l10n related files into moz.build was assumptions in l10n Makefile.in files that various actions occur as part of the "libs" tier. Of course, if we're using a custom variable, we can wire it up to whatever tier/target it needs to enable existing l10n functionality.
Yeah. My thought was to make the recursivemake backend generate as close to exactly the same rules as are there now as possible so that we don't disturb repacks until we actually decide to do that work.
I'm trying to keep things that are not jar.mn packaging outside of the app dirs now, like in bug 1362617 comment 52. I'd be more than happy to make that a strict rule.
Assignee: nobody → ted
OK, I have patches to implement LOCALIZED_FILES and LOCALIZED_PP_FILES in the moz.build frontend and the recursive make backend, and to move firefox-l10n.js into moz.build using them: https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/browser/locales/Makefile.in#31 Running `mach build-backend --diff` with the last changeset results in this diff: https://gist.github.com/luser/d91cd9f1712db4e13fa018327f242289 (plus an unintended diff in the fastermake install manifests, because without special handling these get handled like FINAL_TARGET_FILES!) I'm going to do a little more work on this before getting it up for review, because it turns out that everything l10n touches is a special snowflake, so I need to add more features or I can't actually move any other file handling to moz.build. Specifically, I'll need to handle wildcards (for the hunspell dictionaries), and probably turn this into a FlagsFactory thing to have a way to indicate that crashreporter.ini and crashreporter-override.ini get skipped when building langpacks.
Blocks: 1407374
Here are my WIP changesets if anyone is interested: https://hg.mozilla.org/users/tmielczarek_mozilla.com/mc/shortlog/localized-files-moz-build I think I'm going to finish off the two bits I mentioned in comment 5 (wildcards + a flag for "skip this for langpacks") and then implement something like `LOCALIZED_GENERATED_FILES` in a separate bug to fix a few other bits.
Implementing wildcards was straightforward. Flags, as it turns out, will not be straightforward, because I'm using a `ContextDerivedTypedHierarchicalStringList` (like `FINAL_TARGET_FILES`), but we only have flags for `StrictOrderingOnAppendListWithFlagsFactory`. However! It may not actually matter, since we don't bother with this for updater.ini: https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/browser/locales/Makefile.in#171 ...and the actual langpack packaging code excludes *.ini files: https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/toolkit/locales/l10n.mk#220
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #0) > There are a couple of things that might be a little hard to shoehorn in > here, like searchplugins, which gets the list of filenames to install at > build time: > https://dxr.mozilla.org/mozilla-central/rev/ > 20d57b9c4183973af4af5e078dff2aec0b74f928/browser/locales/Makefile.in#61 FWIW, I looked at this while at a build system work week last week, and mshal and I determined that this doesn't block tup because there's already a `BUILD_FASTER` workaround for the searchplugins stuff: https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/browser/locales/jar.mn#92 We can handle that in a followup when we start changing the actual l10n repack code. (It would be feasible to get the list of searchplugin filenames at configure or build-backend time if we weren't going to reuse the same objdir to repack multiple locales.)
Moving crashreporter.ini and crashreporter-override.ini to LOCALIZED_FILES shows this from `mach build-backend --diff`: https://gist.github.com/luser/47ce8527fd4c4934ccc60aab3f10573d
Attachment #8927984 - Flags: review?(core-build-config-reviews)
Attachment #8927986 - Flags: review?(core-build-config-reviews)
Attachment #8927987 - Flags: review?(core-build-config-reviews)
Attachment #8927988 - Flags: review?(core-build-config-reviews)
I triggered an egregious number of jobs on this try push to try to get builds + repacks on all platforms: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca1e27ffa001567f4c252d4c08ab4a3bdfadc819
That was pretty broken, apparently I don't know how to use INSTALL_TARGETS.
I based those try pushes off of this central changeset which has nightlies and repacks: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=fc194660762d1b92e1679d860a8bf41116d0f54f Assuming everything works on try hopefully I should be able to compare the packages.
Comparing packages turns out to be hard! I neglected to do the extra work to override the build ID for my try push to match, so that differs, and then it turns out the way we do l10n repacks currently is to always pull the tip of each locale's repository, so it's nigh-impossible to reproduce the result of an existing repack. That being said, I wrote a little script to compare artifacts across tasks in two different task groups: https://gist.github.com/luser/e0fc21f40c9a3a1be47c7822926d4552 and ran it against my try push and the nightlies from the base revision on central like so: python compare-build-artifacts.py compare_artifacts bffZM4zhTIi33uAJU5Gk4A KoPSXbDKSteAKbLvQFSW6g 'nightly-l10n-*' '*.xpi' ..and most of the langpacks were identical, so that's comforting. Without being able to use the same locale changeset for each build it's pretty hard to do much better, but it doesn't look like I obviously broke anything.
Comment on attachment 8927986 [details] bug 1409721 - add LOCALIZED_FILES and LOCALIZED_PP_FILES to moz.build sandbox. https://reviewboard.mozilla.org/r/199204/#review204986 This looks sensible. ::: python/mozbuild/mozbuild/frontend/context.py:1398 (Diff revision 2) > + 'LOCALIZED_FILES': (ContextDerivedTypedHierarchicalStringList(Path), list, > + """List of locale-dependent files to be installed into the application > + directory. > + > + This functions similarly to ``FINAL_TARGET_FILES``, but the files are > + sourced from the locale directory and will vary per localization. Be explicit about the "locale directory" -- `LOCALE_SRCDIR`, right? Maybe include how that gets set? ::: python/mozbuild/mozbuild/test/frontend/data/localized-files/moz.build:6 (Diff revision 2) > +# -*- Mode: python; indent-tabs-mode: nil; tab-width: 40 -*- > +# Any copyright is dedicated to the Public Domain. > +# http://creativecommons.org/publicdomain/zero/1.0/ > + > +LOCALIZED_FILES += [ > + 'en-US/bar.ini', Mild preference to include a subdirectory somewhere in here, and to use a wildcard. ::: python/mozbuild/mozbuild/test/frontend/test_emitter.py:1221 (Diff revision 2) > reader = self.reader('final-target-pp-files-non-srcdir') > with self.assertRaisesRegexp(SandboxValidationError, > 'Only source directory paths allowed in FINAL_TARGET_PP_FILES:'): > self.read_topsrcdir(reader) > > + def test_localized_files(self): No PP variant test? Maybe it's to follow.
Attachment #8927986 - Flags: review+
Comment on attachment 8927987 [details] bug 1409721 - wire up support for LOCALIZED[_PP]_FILES in the recursive make backend. https://reviewboard.mozilla.org/r/199206/#review204994 r- only for `AB_CD` question. This is messy, but it was always going to be messy. Consider my comments just that -- comments; I trust your judgement for the final expression of these ideas. ::: commit-message-3dece:7 (Diff revision 2) > + > +This commit adds support for handling LOCALIZED_FILES and LOCALIZED_PP_FILES > +in the recursive make backend. They get special handling because they have > +a few special needs: > +* They run during the libs tier, so that repacks work. > +* The filenames need to be determined at make invocation time, and wrapped "make invocation" isn't very specific. Is that correct? It could be at build time or at package time, so it's truly "make invocation" time? ::: python/mozbuild/mozbuild/backend/recursivemake.py:1464 (Diff revision 2) > backend_file.write('%s_PATH := $(DEPTH)/%s\n' > % (var, mozpath.join(obj.install_target, path))) > backend_file.write('%s_TARGET := misc\n' % var) > backend_file.write('PP_TARGETS += %s\n' % var) > > + def _process_localized_files(self, obj, files, backend_file, preprocessed): Prefer keyword args (`preprocessed=False`) to bare booleans. ::: python/mozbuild/mozbuild/backend/recursivemake.py:1470 (Diff revision 2) > + target = obj.install_target > + path = mozpath.basedir(target, ('dist/bin', )) > + if not path: > + raise Exception('Cannot install localized files to ' + target) > + en_us = 'en-US/' > + for i, (path, files) in enumerate(files.walk()): All of these flags and options are very complicated. This would honestly be easier to follow if it were two functions. If you really want one function, lift all of these widgets out, and include two lines like: ``` # If preprocessed, like "..._FILES += ...". # If not preprocssed, like "..._PP_FILES += ...". ``` As it stands I'd only be able to interpret what you're trying for by inspecting the backend.mk files. ::: python/mozbuild/mozbuild/backend/recursivemake.py:1477 (Diff revision 2) > + self._no_skip['libs'].add(backend_file.relobjdir) > + for f in files: > + if not f.startswith(en_us): > + raise Exception('Localized files not in en-US dir: ' + f) > + f = f[len(en_us):] > + files_type = '' if preprocessed else '_FILES' Do you really want `LOCALIZED_FILES_1_FILES`? ::: python/mozbuild/mozbuild/backend/recursivemake.py:1479 (Diff revision 2) > + if not f.startswith(en_us): > + raise Exception('Localized files not in en-US dir: ' + f) > + f = f[len(en_us):] > + files_type = '' if preprocessed else '_FILES' > + if '*' in f: > + # We can't use MERGE_FILE for wildcards because it takes Can we work around this as a Pre:? It's a weird wart to maintain. ::: python/mozbuild/mozbuild/test/backend/test_recursivemake.py:854 (Diff revision 2) > + env = self._consume('localized-files', RecursiveMakeBackend) > + > + backend_path = mozpath.join(env.topobjdir, 'backend.mk') > + lines = [l.strip() for l in open(backend_path, 'rt').readlines()[2:]] > + > + expected = [ Ah, this is more clear. ::: python/mozbuild/mozbuild/test/backend/test_recursivemake.py:874 (Diff revision 2) > + > + backend_path = mozpath.join(env.topobjdir, 'backend.mk') > + lines = [l.strip() for l in open(backend_path, 'rt').readlines()[2:]] > + > + expected = [ > + 'LOCALIZED_PP_FILES_0 += $(call MERGE_FILE,bar.ini)', How does the PP file get `AB_CD` in this scheme?
Attachment #8927987 - Flags: review-
Comment on attachment 8927988 [details] bug 1409721 - move firefox-l10n.js to moz.build. https://reviewboard.mozilla.org/r/199208/#review205000 r- to revisit `PREFS_DIR`. So many layers of cruft covering this onion :/ ::: browser/locales/Makefile.in (Diff revision 2) > # For Nightly, we know where to get the builds from to do local repacks > ifdef NIGHTLY_BUILD > export EN_US_BINARY_URL ?= https://archive.mozilla.org/pub/firefox/nightly/latest-mozilla-central > endif > > -L10N_PREF_JS_EXPORTS = $(call MERGE_FILE,firefox-l10n.js) Based on https://searchfox.org/mozilla-central/search?q=PREF_DIR&path=, I think that `PREF_DIR` is subtle. This patch doesn't seem to handle that subtlety, and adds a new dependency that isn't reflected in the source code text. Can you explain why you always will have `defaults/preferences` (so not `defaults/prefs`), even when building langpacks? And if this is fine, then let's add a comment connecting `defaults.preferences` to `PREFS_DIR`. Better yet would be to work through all of the details of `PREFS_DIR` and remove it from the system. ::: browser/locales/moz.build:9 (Diff revision 2) > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > JAR_MANIFESTS += ['jar.mn'] > > +LOCALIZED_PP_FILES.defaults.preferences += ['en-US/firefox-l10n.js'] Huh, `firefox-l10n.js` references `AB_CD`, so it must be getting set. Is `AB_CD` passed to all preprocessed files and I just didn't know it?
Attachment #8927988 - Flags: review-
Comment on attachment 8927984 [details] bug 1409721 - move crashreporter.ini and crashreporter-override.ini to moz.build LOCALIZED_FILES. https://reviewboard.mozilla.org/r/199202/#review205004 Again, layers of the onion: why is it okay to do this in langpacks now? ::: toolkit/locales/moz.build:33 (Diff revision 3) > + if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'cocoa': > + LOCALIZED_FILES['crashreporter.app'].Contents.Resources += [ > + 'en-US/crashreporter/crashreporter.ini' > + ] > + else: > + LOCALIZED_FILES += ['en-US/crashreporter/crashreporter.ini'] nit: `[\n...\n]`, to match the list above. Slight preference for extracting the variable or the list into a temporary, so there's only one append.
Attachment #8927984 - Flags: review-
Attachment #8927986 - Flags: review?(core-build-config-reviews)
Attachment #8927987 - Flags: review?(core-build-config-reviews)
Attachment #8927988 - Flags: review?(core-build-config-reviews)
Attachment #8927984 - Flags: review?(core-build-config-reviews)
Comment on attachment 8927988 [details] bug 1409721 - move firefox-l10n.js to moz.build. https://reviewboard.mozilla.org/r/199208/#review205000 > Based on https://searchfox.org/mozilla-central/search?q=PREF_DIR&path=, I think that `PREF_DIR` is subtle. This patch doesn't seem to handle that subtlety, and adds a new dependency that isn't reflected in the source code text. > > Can you explain why you always will have `defaults/preferences` (so not `defaults/prefs`), even when building langpacks? And if this is fine, then let's add a comment connecting `defaults.preferences` to `PREFS_DIR`. > > Better yet would be to work through all of the details of `PREFS_DIR` and remove it from the system. Ah, I had investigated this but neglected to write it down anywhere (to the point where I had to go look at the code again to figure it out *again*). The code in rules.mk uses `defaults/preferences` if either `DIST_SUBDIR` or `XPI_NAME` is set: https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/config/rules.mk#1206 Since this is under browser/, `DIST_SUBDIR` is always set (by way of the export in browser/moz.build), so this doesn't change anything. > Huh, `firefox-l10n.js` references `AB_CD`, so it must be getting set. Is `AB_CD` passed to all preprocessed files and I just didn't know it? Yeah, I thought I was going to have to special-case it but it turns out it's already in `ACDEFINES`: https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/config/config.mk#366 and rules.mk uses `ACDEFINES` when handling `PP_TARGETS`: https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/config/rules.mk#1477
Comment on attachment 8927988 [details] bug 1409721 - move firefox-l10n.js to moz.build. https://reviewboard.mozilla.org/r/199208/#review205000 > Yeah, I thought I was going to have to special-case it but it turns out it's already in `ACDEFINES`: > https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/config/config.mk#366 > > and rules.mk uses `ACDEFINES` when handling `PP_TARGETS`: > https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/config/rules.mk#1477 I'm going to document this better, and also file a followup on making it so `AB_CD` is not in `ACDEFINES` by default.
Comment on attachment 8927984 [details] bug 1409721 - move crashreporter.ini and crashreporter-override.ini to moz.build LOCALIZED_FILES. https://reviewboard.mozilla.org/r/199202/#review205004 I'll mention this in the commit message, but it turns out that langpack packaging excludes ini files: https://bugzilla.mozilla.org/show_bug.cgi?id=1409721#c7
Comment on attachment 8927988 [details] bug 1409721 - move firefox-l10n.js to moz.build. https://reviewboard.mozilla.org/r/199208/#review205000 > Ah, I had investigated this but neglected to write it down anywhere (to the point where I had to go look at the code again to figure it out *again*). The code in rules.mk uses `defaults/preferences` if either `DIST_SUBDIR` or `XPI_NAME` is set: > https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/config/rules.mk#1206 > > Since this is under browser/, `DIST_SUBDIR` is always set (by way of the export in browser/moz.build), so this doesn't change anything. I promised to file a followup to look into whether we can remove `PREFS_DIR`. We'd want to move the equivalent bit of Makefile.in in mobile/locales to moz.build first, and then we'd have to sort out whether the usage in package-manifest.in is actually something that can vary or not.
Comment on attachment 8927988 [details] bug 1409721 - move firefox-l10n.js to moz.build. https://reviewboard.mozilla.org/r/199208/#review205040
Comment on attachment 8927988 [details] bug 1409721 - move firefox-l10n.js to moz.build. https://reviewboard.mozilla.org/r/199208/#review205070 ::: browser/locales/moz.build:9 (Diff revision 2) > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > JAR_MANIFESTS += ['jar.mn'] > > +LOCALIZED_PP_FILES.defaults.preferences += ['en-US/firefox-l10n.js'] I'm not sure if undefining AB_CD by default is going to make our lives easier. It might add more complexity around having to hard-code en-US and then another way to actually have AB_CD.
Comment on attachment 8927986 [details] bug 1409721 - add LOCALIZED_FILES and LOCALIZED_PP_FILES to moz.build sandbox. https://reviewboard.mozilla.org/r/199204/#review205082 ::: python/mozbuild/mozbuild/frontend/context.py:1398 (Diff revision 2) > + 'LOCALIZED_FILES': (ContextDerivedTypedHierarchicalStringList(Path), list, > + """List of locale-dependent files to be installed into the application > + directory. > + > + This functions similarly to ``FINAL_TARGET_FILES``, but the files are > + sourced from the locale directory and will vary per localization. There are two different schemes right now to pick up localized files: One is the legacy way, where we're merrily mixing up English and localized content, and in those the localized files are picked from, in order: - merge dir - localized file - en-US file For fluent-non-legacy, we're having runtime fallback, and we need localized resources to be per language. In this scheme, we're doing: Always keep en-US in the build. For each locale in our build, pick: - merge dir - localized file We're doing "merges" in this case to strip localized strings with errors. It's not really a merge, but its part of the original merge algorithm. ::: python/mozbuild/mozbuild/frontend/context.py:1418 (Diff revision 2) > + ``toolkit/locales/en-US/foo.js`` and > + ``toolkit/locales/en-US/things/*.ini`` to ``$(DIST)/bin/foo`` in an > + en-US build, and in a build of a different locale (or a repack), > + it would copy ``$(LOCALE_SRCDIR)/toolkit/foo.js`` and > + ``$(LOCALE_SRCDIR)/toolkit/things/*.ini``. > + """), I really want us to get rid of this hard-coding of directory patterns in Firefox. It's making mozilla's l10n infrastructure unique in a bad way. I think it's OK to use the en-US file as reference, but please use the logic in compare-locales to find the localized file according to the rules in l10n.toml.
Attachment #8927986 - Flags: review-
Comment on attachment 8927986 [details] bug 1409721 - add LOCALIZED_FILES and LOCALIZED_PP_FILES to moz.build sandbox. https://reviewboard.mozilla.org/r/199204/#review205376 ::: python/mozbuild/mozbuild/frontend/context.py:1418 (Diff revision 2) > + ``toolkit/locales/en-US/foo.js`` and > + ``toolkit/locales/en-US/things/*.ini`` to ``$(DIST)/bin/foo`` in an > + en-US build, and in a build of a different locale (or a repack), > + it would copy ``$(LOCALE_SRCDIR)/toolkit/foo.js`` and > + ``$(LOCALE_SRCDIR)/toolkit/things/*.ini``. > + """), Sorry, but I am not doing cleanup of existing weirdness in this bug. The scope of this bug is simply to hoist these existing files into moz.build so the tup backend has knowledge of them. Changing the behavior of anything is out of scope. We can absolutely make improvements once these changes have landed! ::: python/mozbuild/mozbuild/test/frontend/data/localized-files/moz.build:6 (Diff revision 2) > +# -*- Mode: python; indent-tabs-mode: nil; tab-width: 40 -*- > +# Any copyright is dedicated to the Public Domain. > +# http://creativecommons.org/publicdomain/zero/1.0/ > + > +LOCALIZED_FILES += [ > + 'en-US/bar.ini', I can make this slightly more involved, but I didn't want to go nuts because functionally in the emitter it works almost identically to `FINAL_TARGET_FILES` so there's not a lot of value in exhaustively testing things. ::: python/mozbuild/mozbuild/test/frontend/test_emitter.py:1221 (Diff revision 2) > reader = self.reader('final-target-pp-files-non-srcdir') > with self.assertRaisesRegexp(SandboxValidationError, > 'Only source directory paths allowed in FINAL_TARGET_PP_FILES:'): > self.read_topsrcdir(reader) > > + def test_localized_files(self): I think I didn't write one because we didn't have tests for the PP variants of other similar things (there's really no difference in the emitter handling), but I'll add one just for completeness.
Comment on attachment 8927987 [details] bug 1409721 - wire up support for LOCALIZED[_PP]_FILES in the recursive make backend. https://reviewboard.mozilla.org/r/199206/#review205402 ::: python/mozbuild/mozbuild/backend/recursivemake.py:1464 (Diff revision 2) > backend_file.write('%s_PATH := $(DEPTH)/%s\n' > % (var, mozpath.join(obj.install_target, path))) > backend_file.write('%s_TARGET := misc\n' % var) > backend_file.write('PP_TARGETS += %s\n' % var) > > + def _process_localized_files(self, obj, files, backend_file, preprocessed): Fixed by splitting this into two functions. ::: python/mozbuild/mozbuild/backend/recursivemake.py:1470 (Diff revision 2) > + target = obj.install_target > + path = mozpath.basedir(target, ('dist/bin', )) > + if not path: > + raise Exception('Cannot install localized files to ' + target) > + en_us = 'en-US/' > + for i, (path, files) in enumerate(files.walk()): Yeah, I was originally going to write this as two functions but I merged them when I realized how similar the logic was. But then I slowly found out how many weird differences there are between `INSTALL_TARGETS` and `PP_TARGETS` and it got ugly. I just split them back into two functions, but with a helper function for writing the actual list of files, since that only requires the name to be different, and it makes things fairly straightforward. ::: python/mozbuild/mozbuild/backend/recursivemake.py:1477 (Diff revision 2) > + self._no_skip['libs'].add(backend_file.relobjdir) > + for f in files: > + if not f.startswith(en_us): > + raise Exception('Localized files not in en-US dir: ' + f) > + f = f[len(en_us):] > + files_type = '' if preprocessed else '_FILES' This came out kind of weird. I think originally I had `LOCALIZED_1_FILES`, but then I screwed it up when adding support for the PP variant, so I dunno. `FINAL_TARGET_PP_FILES` get written out as `DIST_FILES_n`, and `OBJDIR_PP_FILES` get written out as `OBJDIR_PP_FILES_n`, but neither of those need the `_FILES` suffix that `INSTALL_TARGETS` require, so maybe it's more confusing than useful. The only weird thing is that if I name it `LOCALIZED_n`, then you wind up writing out `LOCALIZED_n_FILES`, which is OK, but then like `LOCALIZED_n_DEST` which is a little weird. Maybe I'm overthinking this, it's all generated Makefiles that are an implementation detail. I'll just change it. ::: python/mozbuild/mozbuild/backend/recursivemake.py:1479 (Diff revision 2) > + if not f.startswith(en_us): > + raise Exception('Localized files not in en-US dir: ' + f) > + f = f[len(en_us):] > + files_type = '' if preprocessed else '_FILES' > + if '*' in f: > + # We can't use MERGE_FILE for wildcards because it takes I don't have a feasible plan that doesn't involve mucking with repacks, so I'm going to punt on this. Hopefully when we do rework repacks we'll be able to get rid of a lot of this code anyway. ::: python/mozbuild/mozbuild/test/backend/test_recursivemake.py:874 (Diff revision 2) > + > + backend_path = mozpath.join(env.topobjdir, 'backend.mk') > + lines = [l.strip() for l in open(backend_path, 'rt').readlines()[2:]] > + > + expected = [ > + 'LOCALIZED_PP_FILES_0 += $(call MERGE_FILE,bar.ini)', We discussed this on Vidyo yesterday, but config.mk puts `AB_CD` into `ACDEFINES`: https://dxr.mozilla.org/mozilla-central/rev/f41930a869a84af81df1a88d8e82323ff3a6509a/config/config.mk#347 and rules.mk uses `ACDEFINES` for processing `PP_TARGETS`. I added this to the documentation of `LOCALIZED_PP_FILES` in the previous patch.
Comment on attachment 8927987 [details] bug 1409721 - wire up support for LOCALIZED[_PP]_FILES in the recursive make backend. https://reviewboard.mozilla.org/r/199206/#review204994 > "make invocation" isn't very specific. Is that correct? It could be at build time or at package time, so it's truly "make invocation" time? This is definitely confusing as-written! What I mean here is "the filenames cannot be determined at build-backend generation time". I will clarify.
Blocks: 1417931
Blocks: 1417935
I filed bug 1417931 about PREF_DIR and bug 1417935 about AB_CD.
Comment on attachment 8927986 [details] bug 1409721 - add LOCALIZED_FILES and LOCALIZED_PP_FILES to moz.build sandbox. https://reviewboard.mozilla.org/r/199204/#review205082 > There are two different schemes right now to pick up localized files: > > One is the legacy way, where we're merrily mixing up English and localized content, and in those the localized files are picked from, in order: > - merge dir > - localized file > - en-US file > > For fluent-non-legacy, we're having runtime fallback, and we need localized resources to be per language. In this scheme, we're doing: > > Always keep en-US in the build. > > For each locale in our build, pick: > - merge dir > - localized file > > We're doing "merges" in this case to strip localized strings with errors. It's not really a merge, but its part of the original merge algorithm. To echo what ted said, this ticket is only about moving the Makefile.in definitions to moz.build. Sharing the l10n.toml logic with compare-locales is valuable, but it's follow-up: I see that happening when we migrate this stuff from the RecursiveMake backend to the FasterMake backend (which is written in Python and can share the compare-locales implementation.)
Comment on attachment 8927987 [details] bug 1409721 - wire up support for LOCALIZED[_PP]_FILES in the recursive make backend. https://reviewboard.mozilla.org/r/199206/#review205500 ::: commit-message-3dece:12 (Diff revisions 2 - 3) > -* The filenames need to be determined at make invocation time, and wrapped > - with MERGE_FILE so that the file gets picked up from the proper locale dir. > +* The filenames cannot be determined at build-backend generation time, > + since repacks run configure once and then repack multiple locales using > + the generated backend files, so they are written with to be expanded with > + MERGE_FILE by make so that the file gets picked up from the proper locale dir. > + > +Other build backends that aren't trying to handle localized builds will This is a little dicy, but okay. ::: python/mozbuild/mozbuild/backend/recursivemake.py:1467 (Diff revisions 2 - 3) > - for i, (path, files) in enumerate(files.walk()): > - name = 'LOCALIZED%s_FILES_%d' % ('_PP' if preprocessed else '', i) > - self._no_skip['libs'].add(backend_file.relobjdir) > - for f in files: > + for f in files: > - if not f.startswith(en_us): > - raise Exception('Localized files not in en-US dir: ' + f) > + # The emitter asserts that all files start with `en-US/` > + f = f[len('en-US/'):] I quite like `e, f = f.split('en-US/')` and an assertion about `not e` for such patterns. ::: python/mozbuild/mozbuild/test/backend/test_recursivemake.py:855 (Diff revisions 2 - 3) > > backend_path = mozpath.join(env.topobjdir, 'backend.mk') > lines = [l.strip() for l in open(backend_path, 'rt').readlines()[2:]] > > expected = [ > - 'LOCALIZED_FILES_0_FILES += $(wildcard $(LOCALE_SRCDIR)/abc/*.abc)', > + 'LOCALIZED_0_FILES += $(wildcard $(LOCALE_SRCDIR)/abc/*.abc)', I know I said it looked weird... but it had the advantage of making it easier to search (since it's `LOCALIZED_FILES` in input and output). Consider returning to `LOCALIZED_FILES_n_FILES` at your discretion.
Attachment #8927987 - Flags: review?(nalexander) → review+
Comment on attachment 8927988 [details] bug 1409721 - move firefox-l10n.js to moz.build. https://reviewboard.mozilla.org/r/199208/#review205502 ::: commit-message-d2b66:10 (Diff revision 3) > +The extra layer of PREF_DIR can be removed because all of browser > +has DIST_SUBDIR set due to it being exported in browser/moz.build, and > +that means that PREF_DIR is always defaults/preferences here: > +https://dxr.mozilla.org/mozilla-central/rev/f41930a869a84af81df1a88d8e82323ff3a6509a/config/rules.mk#1205 > + > +Additionally, it turns out that PREF_PPFLAGS is not set anywhere in the tree, Dropping this from the tree would be better as a Pre: part. ::: browser/locales/moz.build:9 (Diff revision 3) > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > JAR_MANIFESTS += ['jar.mn'] > > +# If DIST_SUBDIR ever gets unset in browser this path might be wrong. You need to include the magic "PREF_DIR" string so that the person _removing_ PREF_DIR (with https://searchfox.org/mozilla-central/search?q=PREF_DIR&path=) checks this usage.
Attachment #8927988 - Flags: review?(nalexander) → review-
Comment on attachment 8927988 [details] bug 1409721 - move firefox-l10n.js to moz.build. https://reviewboard.mozilla.org/r/199208/#review205504 Gah! That was supposed to be r+ with nits.
Attachment #8927988 - Flags: review- → review+
Comment on attachment 8927984 [details] bug 1409721 - move crashreporter.ini and crashreporter-override.ini to moz.build LOCALIZED_FILES. https://reviewboard.mozilla.org/r/199202/#review205506 ::: toolkit/locales/moz.build:30 (Diff revision 4) > ] > + > +if CONFIG['MOZ_CRASHREPORTER']: > + if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'cocoa': > + # TODO: fixing bug 1223748 should let us remove this special case > + LOCALIZED_FILES['crashreporter.app'].Contents.Resources += [ Same comment about only one assignment as before, although I don't feel too strongly about it.
Attachment #8927984 - Flags: review?(nalexander) → review+
Comment on attachment 8927987 [details] bug 1409721 - wire up support for LOCALIZED[_PP]_FILES in the recursive make backend. https://reviewboard.mozilla.org/r/199206/#review205500 > This is a little dicy, but okay. I know this is true because the files I converted wound up in the FasterMake backend install manifests. :) Actually supporting localized builds / repacks with alternate build backends is going to have to wait until we get the l10n story in a better place, for sure. We could probably support single-locale builds with a non-en-US locale pretty easily, but even that can wait, we don't have a full tup build stood up yet. > I quite like `e, f = f.split('en-US/')` and an assertion about `not e` for such patterns. Oh, that's clever, I'll do that! > I know I said it looked weird... but it had the advantage of making it easier to search (since it's `LOCALIZED_FILES` in input and output). Consider returning to `LOCALIZED_FILES_n_FILES` at your discretion. You're killing me, Smalls!
Comment on attachment 8927988 [details] bug 1409721 - move firefox-l10n.js to moz.build. https://reviewboard.mozilla.org/r/199208/#review205502 > Dropping this from the tree would be better as a Pre: part. I agree, but respectfully decline. :) I don't think removing an empty variable is useful enough of a cleanup to be worth the hassle, especially since we're likely to just remove the other uses of it by migrating them to moz.build after this lands. > You need to include the magic "PREF_DIR" string so that the person _removing_ PREF_DIR (with https://searchfox.org/mozilla-central/search?q=PREF_DIR&path=) checks this usage. That's sensible.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3ce81bc209b09b6771d7056d1fb06a65e27dc0d bug 1409721 - add LOCALIZED_FILES and LOCALIZED_PP_FILES to moz.build sandbox. r=nalexander https://hg.mozilla.org/integration/mozilla-inbound/rev/5ee033a3c356bed86219699698abfe538370740a bug 1409721 - wire up support for LOCALIZED[_PP]_FILES in the recursive make backend. r=nalexander https://hg.mozilla.org/integration/mozilla-inbound/rev/baa2f0ec9b1d2bb0e8c2e55ad38039a364637439 bug 1409721 - move firefox-l10n.js to moz.build. r=nalexander https://hg.mozilla.org/integration/mozilla-inbound/rev/0e485a8fdc3f45ae4cdad75e010765fb48aa20c4 bug 1409721 - move crashreporter.ini and crashreporter-override.ini to moz.build LOCALIZED_FILES. r=nalexander
OK, I can reproduce this locally with an artifact build doing simply `./mach build && ./mach package`. I think the patch in bug 1407374 is actually causing the bustage. It looks like the fastermake backend isn't properly handling the wildcards that patch adds in an install manifest: $ ls /build/artifact-mozilla-central/dist/bin/dictionaries/ *.aff *.dic It literally created directories named `*.aff` and `*.dic` under dist/bin/dictionaries. Humorously, the directories contain symlinks to the proper files: $ ls -l /build/artifact-mozilla-central/dist/bin/dictionaries/* /build/artifact-mozilla-central/dist/bin/dictionaries/*.aff: total 4 lrwxrwxrwx 1 luser luser 77 Nov 20 06:54 en-US.aff -> /build/mozilla-central/extensions/spellcheck/locales/en-US/hunspell/en-US.aff /build/artifact-mozilla-central/dist/bin/dictionaries/*.dic: total 4 lrwxrwxrwx 1 luser luser 77 Nov 20 06:54 en-US.dic -> /build/mozilla-central/extensions/spellcheck/locales/en-US/hunspell/en-US.dic
Flags: needinfo?(ted)
OK, yes, the artifact build on that try push is green. I also figured out the issue that broke them and will file a separate bug for that.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb4ceaf30810fc04b3d0f9a46e2d077b5141af7f bug 1409721 - move crashreporter.ini and crashreporter-override.ini to moz.build LOCALIZED_FILES. r=nalexander https://hg.mozilla.org/integration/mozilla-inbound/rev/338963fe12d3b84dfd6105efa3982e7e34f8ec94 bug 1409721 - move firefox-l10n.js to moz.build. r=nalexander https://hg.mozilla.org/integration/mozilla-inbound/rev/b7c381c2e7f951e36ba421478c6820319bea8caa bug 1409721 - wire up support for LOCALIZED[_PP]_FILES in the recursive make backend. r=nalexander https://hg.mozilla.org/integration/mozilla-inbound/rev/0af8d899db740b9b80f81d51c373ed105ea809d3 bug 1409721 - add LOCALIZED_FILES and LOCALIZED_PP_FILES to moz.build sandbox. r=nalexander
FWIW, the fix for the bustage is in bug 1419055. I'll re-land bug 1407374 on top of that once it's reviewed.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: