Closed Bug 1416891 Opened 7 years ago Closed 7 years ago

Add a LOCALIZED_GENERATED_FILES to the moz.build sandbox

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 2 open bugs)

Details

Attachments

(6 files)

bug 1416891 - add LOCALIZED_GENERATED_FILES to the moz.build sandbox.
59 bytes, text/x-review-board-request
nalexander
: review+
Details
bug 1416891 - allow LOCALIZED_FILES to contain objdir paths as long as they're also from LOCALIZED_GENERATED_FILES.
59 bytes, text/x-review-board-request
nalexander
: review+
Details
bug 1416891 - support LOCALIZED_GENERATED_FILES in the recursive make backend.
59 bytes, text/x-review-board-request
nalexander
: review+
Details
bug 1416891 - use LOCALIZED_GENERATED_FILES for updater.ini.
59 bytes, text/x-review-board-request
nalexander
: review+
Details
bug 1416891 - use LOCALIZED_GENERATED_FILES for update.locale.
59 bytes, text/x-review-board-request
nalexander
: review+
Details
bug 1416891 - Open output files in binary mode for GENERATED_FILES.
59 bytes, text/x-review-board-request
gps
: review+
nalexander
: review+
Details 
bug 1409721 is adding `LOCALIZED_FILES` and `LOCALIZED_PP_FILES` to moz.build. It got involved enough that I'd be happier splitting this work off to a separate bug.

There aren't really many uses of this in desktop Firefox, just updater.ini:
https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/browser/locales/Makefile.in#171

Those could honestly be LOCALIZED_PP_FILES, since all they do is search and replace, but we'd have to modify localized versions of updater.ini to make that work so it doesn't seem particularly worthwhile.

Android has more uses for this, nalexander pointed out his existing `generated_file_template` work:
https://searchfox.org/mozilla-central/source/mobile/android/base/locales/Makefile.in#98-107
I was thinking maybe I would just use `LOCALIZED_PP_FILES` for update.locale, but I think I'll wind up using `LOCALIZED_GENERATED_FILES` for it instead:
https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/toolkit/locales/Makefile.in#24
Comment on attachment 8929608 [details]
bug 1416891 - use LOCALIZED_GENERATED_FILES for updater.ini.

https://reviewboard.mozilla.org/r/200896/#review206422

This, at least, make sense (assuming the earlier stack is correct).

::: browser/locales/generate_updater_ini.py:17
(Diff revision 1)
> +import re
> +
> +def main(output, updater_ini, updater_ini_append, locale=None):
> +    assert(locale is not None)
> +    fixup_re = re.compile('^(Info|Title)Text=')
> +    # updater.ini is always utf-8

nit: full sentences, throughout.
Attachment #8929608 - Flags: review?(nalexander) → review+
Comment on attachment 8929609 [details]
bug 1416891 - use LOCALIZED_GENERATED_FILES for update.locale.

https://reviewboard.mozilla.org/r/200898/#review206426

Shouldn't this just be a `L_PP_FILES`, perhaps with flags to allow the non-standard (WHY?!?!?!) delimiters?  Is this file special (in the l10n repos?) so we can't just stop doing dumb things, and use the standard PP delimiters?  Am I missing the reason for the dumb things here?

::: browser/locales/moz.build:26
(Diff revision 1)
>          '../installer/windows/nsis/updater_append.ini',
>      ]
>      # Yes, this is weird, but what can you do? This file doesn't want to be in the DIST_SUBDIR,
>      # but we can't really move it to a different directory until we change how locale repacks
>      # work.
> -    FINAL_TARGET_FILES['..'] += ['!updater.ini']
> +    LOCALIZED_FILES['..'] += ['!updater.ini']

This should have been in the previous patch, and speaks to a weirdness I pointed out in my review of earlier patches in the stack re: `LOCALIZED_FILES` vs. `FINAL_TARGET_FILES`.
Attachment #8929609 - Flags: review?(nalexander) → review-
Comment on attachment 8929605 [details]
bug 1416891 - add LOCALIZED_GENERATED_FILES to the moz.build sandbox.

https://reviewboard.mozilla.org/r/200890/#review206414

r- because I don't see a path to localized output locations, so I need to know your plan before we push this work.

::: python/mozbuild/mozbuild/frontend/context.py:904
(Diff revision 1)
>                          [('product', unicode), ('component', unicode)])
>  SchedulingComponents = ContextDerivedTypedRecord(
>          ('inclusive', TypedList(unicode, StrictOrderingOnAppendList)),
>          ('exclusive', TypedList(unicode, StrictOrderingOnAppendList)))
>  
> +GeneratedFilesList = StrictOrderingOnAppendListWithFlagsFactory({

This is a Python class, right?  So it's sensible for `G_F` and `L_G_F` to share the _class_ instance, since there's already code in place to ensure all occurrences generate new instances.

Searching and typing this out convinces me this is not an issue, but you could confirm.

::: python/mozbuild/mozbuild/frontend/context.py:1467
(Diff revision 1)
>  
>          Note that the ``AB_CD`` define is available and expands to the current
>          locale being packaged, as with preprocessed entries in jar manifests.
>          """),
>  
> +    'LOCALIZED_GENERATED_FILES': (GeneratedFilesList, list,

I don't see how this will handle localized output locations, but I'll keep reading.

::: python/mozbuild/mozbuild/frontend/context.py:1472
(Diff revision 1)
> +    'LOCALIZED_GENERATED_FILES': (GeneratedFilesList, list,
> +        """Like ``GENERATED_FILES``, but for files whose content varies based on the locale in use.
> +
> +        For simple cases of text substitution, prefer ``LOCALIZED_PP_FILES``.
> +
> +        Refer to the documentation of ``GENERATED_FILES``, for the most part things work the same.

nit: colon or semi-colon instead of comma.
Attachment #8929605 - Flags: review?(nalexander) → review-
Comment on attachment 8929607 [details]
bug 1416891 - support LOCALIZED_GENERATED_FILES in the recursive make backend.

https://reviewboard.mozilla.org/r/200894/#review206416

r- 'cuz I just can't see how this can work, and we need to revisit.

::: commit-message-00aaf:6
(Diff revision 1)
> +bug 1416891 - support LOCALIZED_GENERATED_FILES in the recursive make backend. r?nalexander
> +
> +This change makes the recursive make backend emit slightly different rules
> +when handling LOCALIZED_GENERATED_FILES vs. GENERATED_FILES.
> +
> +First, localized file generation is always done in the libs tier.

Why is this?  I agree that generally, l10n stuff happens in the libs tier.  But that's very inconvenient on Android, and I'd prefer to have some flexibility here.  (I'd like to add a very early java_android tier -- before compile -- that would require l10n stuff.)

::: python/mozbuild/mozbuild/backend/recursivemake.py:548
(Diff revision 1)
> +                if obj.localized:
> +                    # Localized generated files can have locale-specific inputs, which are
> +                    # indicated by paths starting with `en-US/`.
> +                    def srcpath(p):
> +                        if p.startswith('en-US/'):
> +                            e, f = p.split('en-US/')

With the `startswith` check, I think `_, f = ...split()` looks even better.

::: python/mozbuild/mozbuild/backend/recursivemake.py:569
(Diff revision 1)
>              for output in obj.outputs:
>                  if output != first_output:
>                      backend_file.write('%s: %s ;\n' % (output, first_output))
>                  backend_file.write('GARBAGE += %s\n' % output)
>              backend_file.write('EXTRA_MDDEPEND_FILES += %s\n' % dep_file)
>              if obj.script:

Somehow this thing needs to depend on the locale at build-time, and I just don't understand how this can possibly work.  That is, `make -C dir AB_CD=fr` and `make -C dir AB_CD=de` have to both invoke the script, and I don't see how they both can as written.  It's not possible to put a variable like `AB_CD` in Make's rule dependencies, as far as I know.  (Although we do it, and it makes no sense, in many places.)

The only way I was able to make this sensible in `mobile/android` was to FORCE the target and use FileAvoidWrite.

This might require Vidyo to discuss.

::: python/mozbuild/mozbuild/backend/recursivemake.py:572
(Diff revision 1)
>                  backend_file.write('GARBAGE += %s\n' % output)
>              backend_file.write('EXTRA_MDDEPEND_FILES += %s\n' % dep_file)
>              if obj.script:
>                  backend_file.write("""{output}: {script}{inputs}{backend}
>  \t$(REPORT_BUILD)
> -\t$(call py_action,file_generate,{script} {method} {output} $(MDDEPDIR)/{dep_file}{inputs}{flags})
> +\t$(call py_action,file_generate,{locale}{script} {method} {output} $(MDDEPDIR)/{dep_file}{inputs}{flags})

This is a little sensitive because we don't use `--` to terminate arguments to `file_generate`, right?  Not something to fix, just something to be aware of.

::: python/mozbuild/mozbuild/test/backend/test_recursivemake.py:436
(Diff revision 1)
> +            'EXTRA_MDDEPEND_FILES += foo.xyz.pp',
> +            'foo.xyz: %s/generate-foo.py $(call MERGE_FILE,localized-input) $(srcdir)/non-localized-input' % env.topsrcdir,
> +            '$(REPORT_BUILD)',
> +            '$(call py_action,file_generate,--locale=$(AB_CD) %s/generate-foo.py main foo.xyz $(MDDEPDIR)/foo.xyz.pp $(call MERGE_FILE,localized-input) $(srcdir)/non-localized-input)' % env.topsrcdir,
> +            '',
> +            'LOCALIZED_FILES_0_FILES += foo.xyz',

Ah, now I understand why you went to the effort of specially handling objdir files.  Why doesn't `FINAL_TARGET_FILES` just work for this?  And can we get a test that it doesn't work, if it shouldn't be used?  I find this choice mysterious.
Attachment #8929607 - Flags: review?(nalexander) → review-
Comment on attachment 8929606 [details]
bug 1416891 - allow LOCALIZED_FILES to contain objdir paths as long as they're also from LOCALIZED_GENERATED_FILES.

https://reviewboard.mozilla.org/r/200892/#review206428

I think I commented on this, but now I don't see it.  I don't understand why this is necessary; how is it different from `FINAL_TARGET_FILES`?  I'm missing something.

If it is different, we should test that `F_T_F` isn't allowed, so that we don't get two patterns (subtly different?) in the tree.
Attachment #8929606 - Flags: review?(nalexander) → review-
Comment on attachment 8929609 [details]
bug 1416891 - use LOCALIZED_GENERATED_FILES for update.locale.

https://reviewboard.mozilla.org/r/200898/#review206426

This file is not taken from the l10n repos, so we could just change it to `LOCALIZED_PP_FILES` by changing its content. I thought maybe I had a valid reason for doing it this way but I can't recall why! I'll redo that.

> This should have been in the previous patch, and speaks to a weirdness I pointed out in my review of earlier patches in the stack re: `LOCALIZED_FILES` vs. `FINAL_TARGET_FILES`.

Oops, just bad patch management on my part.
Comment on attachment 8929605 [details]
bug 1416891 - add LOCALIZED_GENERATED_FILES to the moz.build sandbox.

https://reviewboard.mozilla.org/r/200890/#review206414

We talked about localized output locations but I did not in fact implement that because nothing in the desktop build needs it. Honestly, I'd prefer to have you come up with a preferred design there, since mobile has existing uses that could use it. If you think there's something that would need extensive changes in my base design here I'm happy to accomodate. Otherwise I think we could implement that as a follow-up to cover the mobile uses.

> This is a Python class, right?  So it's sensible for `G_F` and `L_G_F` to share the _class_ instance, since there's already code in place to ensure all occurrences generate new instances.
> 
> Searching and typing this out convinces me this is not an issue, but you could confirm.

Yeah, the Factory function literally just returns a new class:
https://dxr.mozilla.org/mozilla-central/rev/b056526be38e96b3e381b7e90cd8254ad1d96d9d/python/mozbuild/mozbuild/util.py#609
Comment on attachment 8929607 [details]
bug 1416891 - support LOCALIZED_GENERATED_FILES in the recursive make backend.

https://reviewboard.mozilla.org/r/200894/#review206416

> Why is this?  I agree that generally, l10n stuff happens in the libs tier.  But that's very inconvenient on Android, and I'd prefer to have some flexibility here.  (I'd like to add a very early java_android tier -- before compile -- that would require l10n stuff.)

This is simply because that's what everything currently does, because l10n.mk uses the `libs-%` target:
https://dxr.mozilla.org/mozilla-central/rev/b056526be38e96b3e381b7e90cd8254ad1d96d9d/toolkit/locales/l10n.mk#123

and the browser/locales and mobile/android/locales targets running `make libs` in a bunch of places:
https://dxr.mozilla.org/mozilla-central/rev/b056526be38e96b3e381b7e90cd8254ad1d96d9d/browser/locales/Makefile.in#92
https://dxr.mozilla.org/mozilla-central/rev/b056526be38e96b3e381b7e90cd8254ad1d96d9d/mobile/android/locales/Makefile.in#36

I don't have a great plan for fixing that without diving into "fix repacks".

As written these patches are enough to handle the things that the desktop build does. Obviously the actual tier used here is not hard to change, it's a single line in the recursive make backend. If you've got an idea for how to add more flexibility that doesn't require fiddling with repacks, I'm happy to help!

> With the `startswith` check, I think `_, f = ...split()` looks even better.

Oh, so I tried that and it doesn't work because if the path doesn't start with `en-US` split will only return one value, so you'll get "Too many values to unpack". :-/

> Somehow this thing needs to depend on the locale at build-time, and I just don't understand how this can possibly work.  That is, `make -C dir AB_CD=fr` and `make -C dir AB_CD=de` have to both invoke the script, and I don't see how they both can as written.  It's not possible to put a variable like `AB_CD` in Make's rule dependencies, as far as I know.  (Although we do it, and it makes no sense, in many places.)
> 
> The only way I was able to make this sensible in `mobile/android` was to FORCE the target and use FileAvoidWrite.
> 
> This might require Vidyo to discuss.

Yeah, I think you're right. Looking at the rules in question it looks like they just don't have the output listed at all, so they'll just always run:
https://dxr.mozilla.org/mozilla-central/rev/b056526be38e96b3e381b7e90cd8254ad1d96d9d/browser/locales/Makefile.in#174
https://dxr.mozilla.org/mozilla-central/rev/b056526be38e96b3e381b7e90cd8254ad1d96d9d/toolkit/locales/Makefile.in#24

I'll have to take another look at this. I think we can probably just add some dumb workaround for now, like adding a `FORCE` in repack scenarios.

> This is a little sensitive because we don't use `--` to terminate arguments to `file_generate`, right?  Not something to fix, just something to be aware of.

We don't, but all the arguments here are arguments to `file_generate`. It just calls into the generation method manually, so I don't think there's any risk of the wrong thing eating arguments.

> Ah, now I understand why you went to the effort of specially handling objdir files.  Why doesn't `FINAL_TARGET_FILES` just work for this?  And can we get a test that it doesn't work, if it shouldn't be used?  I find this choice mysterious.

It sorta works, but it runs in the wrong tier. `FINAL_TARGET_FILES` run in `misc` or `export` and this needs to run in `libs`. 

Good point--I added a test for the opposite case, that trying to do `GENERATED_FILES += ['foo'] LOCALIZED_FILES += ['!foo']` will error, but not that using `LOCALIZED_GENERATED_FILES` with `FINAL_TARGET_FILES` should error (and I'm not sure that I made that an error).
Comment on attachment 8929609 [details]
bug 1416891 - use LOCALIZED_GENERATED_FILES for update.locale.

https://reviewboard.mozilla.org/r/200898/#review206426

Oh! I remember now--currently `LOCALIZED_FILES` and `LOCALIZED_PP_FILES` require that the files in question be from the locale srcdir, so I'd have to change that code to allow taking a non-localized file as input, which feels like it could open the door to doing things the wrong way.

Alternately, I realize that I could rewrite this script to not need an input file at all, since the file contents are literally just `$(AB_CD)`, and the script gets that value as an input anyway. I'll do that and see how it looks.
Comment on attachment 8929605 [details]
bug 1416891 - add LOCALIZED_GENERATED_FILES to the moz.build sandbox.

https://reviewboard.mozilla.org/r/200890/#review206414

> I don't see how this will handle localized output locations, but I'll keep reading.

We discussed on Vidyo that nalexander would do some investigation to see whether we can fit localized output paths into this design in a way that would meet Fennec's needs.
Comment on attachment 8929606 [details]
bug 1416891 - allow LOCALIZED_FILES to contain objdir paths as long as they're also from LOCALIZED_GENERATED_FILES.

https://reviewboard.mozilla.org/r/200892/#review206428

We discussed this in the review comments over here:
https://reviewboard.mozilla.org/r/200894/#review206416

I'm going to make it an error to use `LOCALIZED_GENERATED_FILES` from the objdir in anything *but* `LOCALIZED_FILES` and a test for that.
Comment on attachment 8929607 [details]
bug 1416891 - support LOCALIZED_GENERATED_FILES in the recursive make backend.

https://reviewboard.mozilla.org/r/200894/#review207358

::: commit-message-00aaf:6
(Diff revision 1)
> +bug 1416891 - support LOCALIZED_GENERATED_FILES in the recursive make backend. r?nalexander
> +
> +This change makes the recursive make backend emit slightly different rules
> +when handling LOCALIZED_GENERATED_FILES vs. GENERATED_FILES.
> +
> +First, localized file generation is always done in the libs tier.

We touched on this a little bit, but I don't think this is fixable as l10n repacks currently stand.

::: python/mozbuild/mozbuild/test/backend/test_recursivemake.py:436
(Diff revision 1)
> +            'EXTRA_MDDEPEND_FILES += foo.xyz.pp',
> +            'foo.xyz: %s/generate-foo.py $(call MERGE_FILE,localized-input) $(srcdir)/non-localized-input' % env.topsrcdir,
> +            '$(REPORT_BUILD)',
> +            '$(call py_action,file_generate,--locale=$(AB_CD) %s/generate-foo.py main foo.xyz $(MDDEPDIR)/foo.xyz.pp $(call MERGE_FILE,localized-input) $(srcdir)/non-localized-input)' % env.topsrcdir,
> +            '',
> +            'LOCALIZED_FILES_0_FILES += foo.xyz',

I added a test in the previous patch.
Comment on attachment 8929607 [details]
bug 1416891 - support LOCALIZED_GENERATED_FILES in the recursive make backend.

https://reviewboard.mozilla.org/r/200894/#review206416

> Yeah, I think you're right. Looking at the rules in question it looks like they just don't have the output listed at all, so they'll just always run:
> https://dxr.mozilla.org/mozilla-central/rev/b056526be38e96b3e381b7e90cd8254ad1d96d9d/browser/locales/Makefile.in#174
> https://dxr.mozilla.org/mozilla-central/rev/b056526be38e96b3e381b7e90cd8254ad1d96d9d/toolkit/locales/Makefile.in#24
> 
> I'll have to take another look at this. I think we can probably just add some dumb workaround for now, like adding a `FORCE` in repack scenarios.

As we discussed, I fixed this by adding a `$(if $(IS_LANGUAGE_REPACK),FORCE)` to the dependencies of the generated file, so that it will be forcibly rebuilt in repacks but not otherwise.
I addressed all the review comments from the previous review. The current plan is to get these patches reviewed, and for nalexander to determine whether supporting localized output file paths will require changes to the design here, or whether it can be done as a followup.

For sanity's sake, here's the output of `mach build-backend --diff` for the sum of these changesets:
https://gist.github.com/luser/e43265cd04cf013f3e9be4c1648ee795
Comment on attachment 8929605 [details]
bug 1416891 - add LOCALIZED_GENERATED_FILES to the moz.build sandbox.

https://reviewboard.mozilla.org/r/200890/#review207618
Attachment #8929605 - Flags: review?(nalexander) → review+
Comment on attachment 8929606 [details]
bug 1416891 - allow LOCALIZED_FILES to contain objdir paths as long as they're also from LOCALIZED_GENERATED_FILES.

https://reviewboard.mozilla.org/r/200892/#review207620

::: python/mozbuild/mozbuild/frontend/emitter.py:1151
(Diff revisions 1 - 2)
> +                            if f.target_basename not in localized_generated_files:
> -                            raise SandboxValidationError(
> +                                raise SandboxValidationError(
> -                                ('Objdir file listed in %s not in ' +
> +                                    ('Objdir file listed in %s not in ' +
> -                                 'LOCALIZED_GENERATED_FILES: %s') % (var, f), context)
> +                                     'LOCALIZED_GENERATED_FILES: %s') % (var, f), context)
> +                        else:
> +                            # Additionally, don't allow LOCALIZED_GENERATED_FILES to be used

Say why we don't allow this, here or in the commit message (or both).
Attachment #8929606 - Flags: review?(nalexander) → review+
Comment on attachment 8929607 [details]
bug 1416891 - support LOCALIZED_GENERATED_FILES in the recursive make backend.

https://reviewboard.mozilla.org/r/200894/#review206416

> Oh, so I tried that and it doesn't work because if the path doesn't start with `en-US` split will only return one value, so you'll get "Too many values to unpack". :-/

But you have the `startswith` check, so you're guaranteed to have a split, and this asserts you have exactly one.  It's not a big deal.
Comment on attachment 8929607 [details]
bug 1416891 - support LOCALIZED_GENERATED_FILES in the recursive make backend.

https://reviewboard.mozilla.org/r/200894/#review207624

r+ so you don't need to go through a cycle with me.  Once you're happy with the form of the dependency, bombs away.

::: python/mozbuild/mozbuild/test/backend/test_recursivemake.py:430
(Diff revisions 1 - 2)
>          backend_path = mozpath.join(env.topobjdir, 'backend.mk')
>          lines = [l.strip() for l in open(backend_path, 'rt').readlines()[2:]]
>  
>          expected = [
>              'libs:: foo.xyz',
> +            'foo.xyz: $(if $(IS_LANGUAGE_REPACK),FORCE)',

Doesn't make do weird things with empty dependencies, such that you actually want `;` and not ``?  (I haven't fully internalized these weird things.)
Attachment #8929607 - Flags: review?(nalexander) → review+
Comment on attachment 8929609 [details]
bug 1416891 - use LOCALIZED_GENERATED_FILES for update.locale.

https://reviewboard.mozilla.org/r/200898/#review207626

Nice!  This piece definitely got better.
Attachment #8929609 - Flags: review?(nalexander) → review+
Comment on attachment 8929607 [details]
bug 1416891 - support LOCALIZED_GENERATED_FILES in the recursive make backend.

https://reviewboard.mozilla.org/r/200894/#review207624

> Doesn't make do weird things with empty dependencies, such that you actually want `;` and not ``?  (I haven't fully internalized these weird things.)

Oh, you're probably right! I will change this to put this dependency inline with the other dependencies to avoid this problem.
Comment on attachment 8933363 [details]
bug 1416891 - Open output files in binary mode for GENERATED_FILES.

https://reviewboard.mozilla.org/r/204274/#review209864

If it's green on try, it's sensible to me, but you might want gps's stamp.
Attachment #8933363 - Flags: review+
Attachment #8933363 - Flags: review?(core-build-config-reviews) → review?(gps)
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #33)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=203d8aa7ae1ebfa5e70fa85a7b60e53eca863034

This was broken on Windows because I forgot to `import shutil` in the one script, and that was only used on Windows. While re-testing it locally I noticed that the updater.ini from my local build didn't match the one from my Nightly build because the line endings were different. With the last patch here applied the updater.ini files are identical. file_generate.py was opening the output file in text mode, so it'd write CRLF endings on Windows, where the previous Makefile rules would just run the in-tree updater.ini (which has LF endings) through sed so the generated file would also have LF endings.

I don't *think* this should matter very much to anything else, but I am going to do another try push just to make sure.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #42)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=693c8932e202f297763143b0d7f64d4e4ab9b0dd

This try push looks good. I also spot-checked some of the l10n repacks from the try push in comment 26 and they appear correct.
Comment on attachment 8933363 [details]
bug 1416891 - Open output files in binary mode for GENERATED_FILES.

https://reviewboard.mozilla.org/r/204274/#review210728

I trust you can fix the issue before landing.

::: python/mozbuild/mozbuild/action/file_generate.py:67
(Diff revision 1)
>                file=sys.stderr)
>          return 1
>  
>      ret = 1
>      try:
> -        with FileAvoidWrite(args.output_file) as output:
> +        with FileAvoidWrite(args.output_file, mode='rb') as output:

This should be `wb`. I'm kinda surprised this doesn't raise an error!
Attachment #8933363 - Flags: review?(gps) → review+
Comment on attachment 8933363 [details]
bug 1416891 - Open output files in binary mode for GENERATED_FILES.

https://reviewboard.mozilla.org/r/204274/#review210728

> This should be `wb`. I'm kinda surprised this doesn't raise an error!

I mentioned this on IRC, but I'll record it here for posterity. This is just weirdness in the `FileAvoidWrite` API. It uses the specified mode to open any existing file, but then converts it to a write mode for the actual writing:
https://dxr.mozilla.org/mozilla-central/rev/709f355a7a8c4ae426d1824841a71ffdb5ce0137/python/mozbuild/mozbuild/util.py#264

It seems like we could make that less confusing, for sure.
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc1de75babe66108049cd6c925098da8967a3fa2
bug 1416891 - add LOCALIZED_GENERATED_FILES to the moz.build sandbox. r=nalexander

https://hg.mozilla.org/integration/mozilla-inbound/rev/95a36c00b7e08d68b0c536732e46a504c8ff84c4
bug 1416891 - allow LOCALIZED_FILES to contain objdir paths as long as they're also from LOCALIZED_GENERATED_FILES. r=nalexander

https://hg.mozilla.org/integration/mozilla-inbound/rev/04ab61857a9e1beb7da3b5f921d31fc37d860b6f
bug 1416891 - support LOCALIZED_GENERATED_FILES in the recursive make backend. r=nalexander

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e56a93c721ae998dc16a4ae672bf8189f3f1086
bug 1416891 - use LOCALIZED_GENERATED_FILES for updater.ini. r=nalexander

https://hg.mozilla.org/integration/mozilla-inbound/rev/785c8a991932b93b3b96c6d34d5fedc88d5278d0
bug 1416891 - use LOCALIZED_GENERATED_FILES for update.locale. r=nalexander

https://hg.mozilla.org/integration/mozilla-inbound/rev/47be09a53f3df6e2beab0a8638c0f108de4454f6
bug 1416891 - Open output files in binary mode for GENERATED_FILES. r=nalexander,gps
Blocks: 1423198
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: