Closed Bug 1447932 Opened 8 years ago Closed 7 years ago

Make localized artifact builds work

Categories

(Firefox Build System :: General, enhancement)

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

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: ted, Assigned: ted)

Details

Attachments

(1 file, 2 obsolete files)

bug 1447932 - Add support for localized files to fastermake backend.
59 bytes, text/x-review-board-request
mshal
: review+
Details
We talked about this in a meeting in Austin as one of the things we could do that would simplify life for localizers. I took a crack at it and I got something working without a huge amount of effort: https://hg.mozilla.org/users/tmielczarek_mozilla.com/mc/rev/localized-artifact-build I don't think it's 100% correct yet, in that it might not be handling `LOCALIZED_GENERATED_FILES` and I know for sure it's not handling the searchplugins bits that are still currently generated by Makefile.in rules (bug 1437942 covers changing that), but it definitely gets me a build with a localized UI from `./mach build; ./mach run`. I checked out the es-ES localization to /build/es-ES and used the following mozconfig: mk_add_options MOZ_OBJDIR=/build/opt-es-firefox mk_add_options AUTOCLOBBER=1 ac_add_options --enable-artifact-builds ac_add_options --with-l10n-base=/build/ ac_add_options --enable-ui-locale=es-ES
Actually it looks like `LOCALIZED_GENERATED_FILES` works, but not `LOCALIZED_FILES`.
Interesting. I think that --enable-ui-locale is broken outside of artifact builds, too, I wanted to actually remove that option. I morphed bug 1402564 into that.
I noticed that error when I originally tried building without `--enable-artifact-builds`. Not supporting full locale builds seems fine, but if we can make localized artifact builds work they seem like a useful thing to support. I'd also like to look into having a simpler way to just build a langpack without jumping through all the current hoops.
Axel - is there a way to reduce the difference between "rebuild" and "repack" to non significant? It seems to me that if our build system became smart enough to react to this scenario: ``` ./mach build --moz-ui-locales = pl ./mach build --moz-ui-locales = de,fr ``` (or its equivalent in mozconfig) by skipping through everything else, just like `./mach build faster` does and only repackage the locale bits, you'd get comparable UX as we have with repacks. Is there something I'm missing?
Flags: needinfo?(l10n)
I'm not sure there is, and definitely not with ./mach build, as that forwards to the build system aka make, and make doesn't take arguments like that.
Flags: needinfo?(l10n)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #5) > Axel - is there a way to reduce the difference between "rebuild" and > "repack" to non significant? This is my ideal end state. An artifact build and a repack are extremely similar--they both involve downloading a build, unpacking it and then taking some files from the source and inserting them into the build. The difference is that the logic in the artifact build is way easier to follow and doesn't involve the recursive make build hardly at all.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #4) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=ab95234e4948e304c032f665b72b7d7313b32900 I apparently had not tested an en-US artifact build and I had a bug in one of the code paths there. Oops. I fixed that and tested it locally and it worked, so this try push should be better.
Comment on attachment 8975544 [details] bug 1447932 - Add support for localized files to fastermake backend. https://reviewboard.mozilla.org/r/243802/#review251958 ::: python/mozbuild/mozbuild/backend/common.py:422 (Diff revision 1) > files = jar_context['FINAL_TARGET_FILES'] > files_pp = jar_context['FINAL_TARGET_PP_FILES'] > > for e in jarinfo.entries: > if e.is_locale: > - if jarinfo.relativesrcdir: > + src = '/' + self.localized_path(jarinfo.relativesrcdir or obj.relsrcdir, Why do you need to explicitly prefix with a '/'? It looks like self.localized_path() either returns something with topsrcdir or L10NBASEDIR as the first component, which I think are already full paths.
Attachment #8975544 - Flags: review+
Attachment #8975544 - Flags: review?(core-build-config-reviews)
Comment on attachment 8975544 [details] bug 1447932 - Add support for localized files to fastermake backend. https://reviewboard.mozilla.org/r/243802/#review254064 ::: python/mozbuild/mozbuild/backend/common.py:422 (Diff revision 1) > files = jar_context['FINAL_TARGET_FILES'] > files_pp = jar_context['FINAL_TARGET_PP_FILES'] > > for e in jarinfo.entries: > if e.is_locale: > - if jarinfo.relativesrcdir: > + src = '/' + self.localized_path(jarinfo.relativesrcdir or obj.relsrcdir, You're right, but this path gets passed into the `Path` constructor below, so paths with a single leading / will be treated as topsrcdir-relative paths. We need to have // to be considered a real absolute path (which we do need because `L10NBASEDIR` is outside the srcdir).
Attachment #8975544 - Attachment is obsolete: true
Attachment #8975545 - Attachment is obsolete: true
Sorry about the shuffling but I wanted to land this and realized that only half of it had been reviewed, so I squashed the patches. I had to add a small fix to generate_builtin_addons.py (which I know has like two outstanding patches in bugzilla as well) to handle building a locale that doesn't have any dictionaries (dist/bin/dictionaries doesn't get populated in that case).
I broke some of the `test_faster_make` tests on that push: [task 2018-06-22T16:34:53.972Z] 16:34:53 INFO - > ab_cd = obj.config.substs['MOZ_UI_LOCALE'][0] [task 2018-06-22T16:34:53.972Z] 16:34:53 INFO - E KeyError: u'MOZ_UI_LOCALE' I just need to make the config environment they use set that, presumably.
OK, that last try push looks good. mshal: if you r+ this, can you autoland it as well? Thanks!
Comment on attachment 8987030 [details] bug 1447932 - Add support for localized files to fastermake backend. https://reviewboard.mozilla.org/r/252276/#review259242 ::: python/mozbuild/mozbuild/action/generate_builtin_addons.py:28 (Diff revision 1) > > bindir = os.path.join(buildconfig.topobjdir, 'dist/bin') > > def find_dictionaries(path): > dicts = {} > - for filename in os.listdir(os.path.join(bindir, path)): > + bin_path = os.path.join(bindir, path) With generate_builtin_addons.py changing in bug 1459004, this part should no longer be required (the listdir stuff is going away). Just a heads up in case that lands first. ::: python/mozbuild/mozbuild/action/l10n_merge.py:19 (Diff revision 1) > +) > + > + > +def main(argv): > + parser = argparse.ArgumentParser(description='Merge l10n files.') > + parser.add_argument('--output', help='File patterns to exclude') It looks like the help messages for these three arguments were copied from symbols_archive.py and need to be updated. ::: python/mozbuild/mozbuild/action/l10n_merge.py:38 (Diff revision 1) > + cc.compare(File(args.ref_file, args.ref_file, ''), > + File(args.l10n_file, args.l10n_file, ''), > + args.output) > + > + ensureParentDir(args.output) > + if not os.path.exists(args.output): Why do we not want to do anything if the output already exists? Shouldn't we overwrite it with the new file if one of the inputs has changed? Eg: If we were using the ref_file before and added a new l10n_file, the output should now be the l10n_file. ::: python/mozbuild/mozbuild/backend/fastermake.py:75 (Diff revision 1) > for f in files: > + # For localized files we need to find the file from the locale directory. > + if localized and not isinstance(f, ObjDirPath): > + src = self.localized_path(obj.relsrcdir, f) > + > + # TODO: don't do this for en-US. Does this need to be addressed before landing? ::: python/mozbuild/mozbuild/backend/fastermake.py:81 (Diff revision 1) > + dep_target = 'install-%s' % obj.install_target > + > + if '*' not in src: > + import os > + merge = mozpath.join('l10n_merge', obj.relsrcdir, f) > + merge = os.path.abspath(mozpath.join(self.environment.topobjdir, merge)) mozpath also has an abspath, so instead of importing os here we could just do: merge = mozpath.abspath(mozpath.join(self.environment.topobjdir, 'l10n_merge', obj.relsrcdir, f)) ::: python/mozbuild/mozbuild/backend/fastermake.py:183 (Diff revision 1) > > + # Add l10n dependencies we inferred: > + # (target, {'d1':(ref, l10n), 'd2':(ref, l10n)}) > + # TODO: handle wildcards? > + for target, deps in self._l10n_dependencies.iteritems(): > + mk.create_rule([target]).add_dependencies( Do we want to create dummy targets here for files that don't exist? When I tried this with the de locale, it doesn't have aboutStudies.properties, which resulted in: make: \*\*\* No rule to make target '/home/mshal/l10n/de/toolkit/chrome/global/aboutStudies.properties', needed by '/home/mshal/mozilla-central-tmp/obj-es-es/l10n_merge/toolkit/locales/en-US/chrome/global/aboutStudies.properties'. Stop. From l10n_merge.py it looks like this should not fail, but instead default to the ref_file. ::: python/mozbuild/mozbuild/backend/fastermake.py:189 (Diff revision 1) > + '%s' % d[0] for d in deps) > + for (merge, ref_file, l10n_file) in deps: > + print((merge, ref_file, l10n_file)) > + rule = mk.create_rule([merge]).add_dependencies( > + [ref_file, l10n_file]) > + rule.add_commands(['$(PYTHON) -m mozbuild.action.l10n_merge --output {} --ref-file {} --l10n-file {}'.format(merge, ref_file, l10n_file)]) Should we use the file_generate action here? Right now the python files aren't listed as dependencies, so for example changing l10n_merge.py doesn't cause these files to rebuild. If using file_generate is a pain, we could try adding l10n_merge.py and its dependencies manually in add_dependencies() above.
Attachment #8987030 - Flags: review?(mshal)
Hi Ted Would it be possible for you to refactor the patches to address Mike's suggestions so we can close out this issue? Kim
Flags: needinfo?(ted)
Comment on attachment 8987030 [details] bug 1447932 - Add support for localized files to fastermake backend. https://reviewboard.mozilla.org/r/252276/#review259242 > With generate_builtin_addons.py changing in bug 1459004, this part should no longer be required (the listdir stuff is going away). Just a heads up in case that lands first. Thanks! I'll drop that bit and rebase over that change. > It looks like the help messages for these three arguments were copied from symbols_archive.py and need to be updated. Oops, good catch! > Why do we not want to do anything if the output already exists? Shouldn't we overwrite it with the new file if one of the inputs has changed? Eg: If we were using the ref_file before and added a new l10n_file, the output should now be the l10n_file. the call to `cc.compare` will write the output file, but the l10n merge code was weird. Pike fixed that in bug 1469337 just recently, so this could be simplified. Previously l10n merge wouldn't write the output file if there was no merging to do (if it just wanted to copy one of the inputs directly to the output), so we had to check for that case and handle it. If I rebase over his changes I should be able to drop this block entirely. > Does this need to be addressed before landing? Yes, good point. > mozpath also has an abspath, so instead of importing os here we could just do: > > merge = mozpath.abspath(mozpath.join(self.environment.topobjdir, 'l10n_merge', obj.relsrcdir, f)) Leftovers from hacky prototyping. :) > Do we want to create dummy targets here for files that don't exist? When I tried this with the de locale, it doesn't have aboutStudies.properties, which resulted in: > > make: \*\*\* No rule to make target '/home/mshal/l10n/de/toolkit/chrome/global/aboutStudies.properties', needed by '/home/mshal/mozilla-central-tmp/obj-es-es/l10n_merge/toolkit/locales/en-US/chrome/global/aboutStudies.properties'. Stop. > > From l10n_merge.py it looks like this should not fail, but instead default to the ref_file. Oh, good point! We didn't hit this with the locales we used in testing. > Should we use the file_generate action here? Right now the python files aren't listed as dependencies, so for example changing l10n_merge.py doesn't cause these files to rebuild. If using file_generate is a pain, we could try adding l10n_merge.py and its dependencies manually in add_dependencies() above. Hm, yeah. I think we can probably use file_generate and just convert this l10n_merge script to match. I'll give that a shot.
Working on that right now.
Flags: needinfo?(ted)
Comment on attachment 8987030 [details] bug 1447932 - Add support for localized files to fastermake backend. https://reviewboard.mozilla.org/r/252276/#review259242 > Hm, yeah. I think we can probably use file_generate and just convert this l10n_merge script to match. I'll give that a shot. Actually, I think this might be more of a pain than it's worth right now because file_generate wants to write out a .pp file for dependencies, but the fastermake backend doesn't have the bits to deal with that directly. I'll just hardcode some extra dependencies on the Python scripts and we'll call it good enough for now.
I updated the patch to address your review comments, but I didn't rebase it yet. It should be easier to read the interdiff on this patch. I'm rebasing it now and will push the rebased version up here as well afterwards.
Oh, I missed the fact that bug 1469337 only made that change in the standalone compare-locales repo, it didn't get re-vendored into m-c yet.
Ok, that patch is the rebased version.
Comment on attachment 8987030 [details] bug 1447932 - Add support for localized files to fastermake backend. https://reviewboard.mozilla.org/r/252276/#review264842
Attachment #8987030 - Flags: review?(mshal) → review+
Comment on attachment 8987030 [details] bug 1447932 - Add support for localized files to fastermake backend. https://reviewboard.mozilla.org/r/252276/#review264850 As mentioned in the conversation in San Francisco, the way compare-locales is called here is not going to work, due to the way it needs to be configured. We should do this work once the uplift of compare-locales release to m-c is done, and we can rely on the l10n merge dir to be complete.
Attachment #8987030 - Flags: review-
We're not intending to support this fully right now, we'd just like to land this work because it's potentially useful and informative for future work. As long as it mostly works and doesn't break other things I think it's fine if it has some rough edges. We can either improve it when we improve other l10n bits or drop it in favor of other workflows if those wind up being better.
I'm optimistic that we'll have a release of compare-locales to uplift to m-c next week, and then there's code in the tree that is written will full testing that it's creating the right files. I'm not sure the patch in this bug handles fluent files right, for example. Here's what I suggest the dependencies to be with a full merge dir: If you're doing en-US, the file mappings are from jar.mn, dependencies as given. If you're doing ab-CD, the file mappings are from jar.mn into l10n-merge/ab-CD, dependencies as given. compare-locales is run unconditionally, regenerates all the files, all localized assets get rebuilt. That's slightly less optimized than what's proposed in this patch, but for most use-cases, I think that's practically what's happening even with fully optimized dependencies. Even for someone that edits a localization and rebuilds to test that will get 15 secs build times or so, that's OK?
Pushed by tmielczarek@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f19f8d717c6b Add support for localized files to fastermake backend. r=mshal
Backed out for build bustages on test_fastermake. Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&fromchange=f19f8d717c6bb7497b6242a860fd96d8d82c50b7&tochange=1bb77a767731a18e4c63de6d69e2997fb3c1b577&selectedJob=189506113 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=189506113&repo=autoland&lineNumber=41958 Backout link: https://hg.mozilla.org/integration/autoland/rev/1bb77a767731a18e4c63de6d69e2997fb3c1b577 [task 2018-07-23T11:36:14.877Z] 11:36:14 INFO - /builds/worker/workspace/build/src/python/mozbuild/mozbuild/test/backend/test_fastermake.py [task 2018-07-23T11:36:14.877Z] 11:36:14 INFO - ============================= test session starts ============================== [task 2018-07-23T11:36:14.879Z] 11:36:14 INFO - platform linux2 -- Python 2.7.9, pytest-3.6.2, py-1.5.4, pluggy-0.6.0 -- /builds/worker/workspace/build/src/obj-firefox/_virtualenvs/src-UL-dti-o-2.7/bin/python [task 2018-07-23T11:36:14.879Z] 11:36:14 INFO - rootdir: /builds/worker/workspace/build/src, inifile: /builds/worker/workspace/build/src/config/mozunit/mozunit/pytest.ini [task 2018-07-23T11:36:14.880Z] 11:36:14 INFO - collecting ... collected 2 items [task 2018-07-23T11:36:14.881Z] 11:36:14 INFO - ../python/mozbuild/mozbuild/test/backend/test_fastermake.py::TestFasterMakeBackend::test_basic PASSED [task 2018-07-23T11:36:14.881Z] 11:36:14 WARNING - ../python/mozbuild/mozbuild/test/backend/test_fastermake.py::TestFasterMakeBackend::test_final_target_files_wildcard TEST-UNEXPECTED-FAIL [task 2018-07-23T11:36:14.882Z] 11:36:14 INFO - =================================== FAILURES =================================== [task 2018-07-23T11:36:14.882Z] 11:36:14 INFO - ____________ TestFasterMakeBackend.test_final_target_files_wildcard ____________ [task 2018-07-23T11:36:14.883Z] 11:36:14 INFO - self = <mozbuild.test.backend.test_fastermake.TestFasterMakeBackend testMethod=test_final_target_files_wildcard> [task 2018-07-23T11:36:14.884Z] 11:36:14 INFO - def test_final_target_files_wildcard(self): [task 2018-07-23T11:36:14.884Z] 11:36:14 INFO - """Ensure that wildcards in FINAL_TARGET_FILES work properly.""" [task 2018-07-23T11:36:14.885Z] 11:36:14 INFO - > env = self._consume('final-target-files-wildcard', FasterMakeBackend) [task 2018-07-23T11:36:14.885Z] 11:36:14 INFO - ../python/mozbuild/mozbuild/test/backend/test_fastermake.py:34: [task 2018-07-23T11:36:14.886Z] 11:36:14 INFO - _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ [task 2018-07-23T11:36:14.886Z] 11:36:14 INFO - ../python/mozbuild/mozbuild/test/backend/common.py:257: in _consume [task 2018-07-23T11:36:14.887Z] 11:36:14 INFO - backend.consume(objs) [task 2018-07-23T11:36:14.887Z] 11:36:14 INFO - ../python/mozbuild/mozbuild/backend/base.py:128: in consume [task 2018-07-23T11:36:14.888Z] 11:36:14 INFO - if (not self.consume_object(obj) and [task 2018-07-23T11:36:14.888Z] 11:36:14 INFO - _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ [task 2018-07-23T11:36:14.889Z] 11:36:14 INFO - self = <mozbuild.backend.fastermake.FasterMakeBackend object at 0x7f52ed6c1c50> [task 2018-07-23T11:36:14.890Z] 11:36:14 INFO - obj = <mozbuild.frontend.data.FinalTargetFiles object at 0x7f52ed6d62d0> [task 2018-07-23T11:36:14.891Z] 11:36:14 INFO - def consume_object(self, obj): [task 2018-07-23T11:36:14.891Z] 11:36:14 INFO - if isinstance(obj, JARManifest) and \ [task 2018-07-23T11:36:14.892Z] 11:36:14 INFO - obj.install_target.startswith('dist/bin'): [task 2018-07-23T11:36:14.892Z] 11:36:14 INFO - self._consume_jar_manifest(obj) [task 2018-07-23T11:36:14.893Z] 11:36:14 INFO - elif isinstance(obj, (FinalTargetFiles, [task 2018-07-23T11:36:14.893Z] 11:36:14 INFO - FinalTargetPreprocessedFiles)) and \ [task 2018-07-23T11:36:14.894Z] 11:36:14 INFO - obj.install_target.startswith('dist/bin'): [task 2018-07-23T11:36:14.894Z] 11:36:14 INFO - > ab_cd = self.environment.substs['MOZ_UI_LOCALE'][0] [task 2018-07-23T11:36:14.895Z] 11:36:14 INFO - E KeyError: u'MOZ_UI_LOCALE' [task 2018-07-23T11:36:14.895Z] 11:36:14 INFO - ../python/mozbuild/mozbuild/backend/fastermake.py:65: KeyError
Flags: needinfo?(ted)
Hm, I pushed to try but maybe it didn't run these tests for some reason? I'll fix the tests and see if I can get a more thorough try push.
Flags: needinfo?(ted)
This must have just been an unfortunate rebase over a changeset that added that test.
(In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #39) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=6b4b259f738a6ad51b344c1365c09e78b72dda37 This is green, and this seemed to be the only failing test, so I'm going to re-land this.
Pushed by tmielczarek@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/789b4ff72fff Add support for localized files to fastermake backend. r=mshal
https://hg.mozilla.org/mozilla-central/rev/789b4ff72fff
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: