Closed Bug 1249912 Opened 4 years ago Closed 4 years ago

Single-locale android repacks broken with: AndroidManifest.xml:2: error: Error: String types not allowed (at 'versionCode' with value '').

Categories

(Firefox Build System :: Android Studio and Gradle Integration, defect)

defect
Not set

Tracking

(firefox47 fixed)

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: Pike, Assigned: mshal)

References

Details

Attachments

(1 file)

https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&filter-job_group_symbol=L10n&filter-job_group_symbol=Update-3&filter-job_group_symbol=Update-1&filter-job_group_symbol=Update-2&exclusion_profile=false&fromchange=d1a54ae63da7 gives me consistently broken single-locale android builds on central.

They were busted prior due to the tooltool stuff, so that regression window isn't really solid.

There is the landing of "bug 1074258 - Expand entities at build time when copying strings.xml in Gradle" in that window, though.

Nick, do you have an idea?
Flags: needinfo?(nalexander)
What's happening is that the preprocessor is failing to produce AndroidManifest.xml due to a bad Android version code.  A little digging reveals:

04:45:04     INFO -  awk: cmd. line:1: fatal: cannot open file `../../../buildid.h' for reading (No such file or directory)

glandium had a bunch of build changes around buildid.h, and those changed the fragile set of conditions required to make the tree produce Android repack APKs.   Those changes landed a while back and were masked by other bustage.

glandium, can you link to that ticket; and can you suggest how to ensure buildid.h is in place for these repacks?  Thanks!
Flags: needinfo?(nalexander) → needinfo?(mh+mozilla)
Blocks: 1246881
Flags: needinfo?(mh+mozilla)
Looks like mobile_l10n.py is expecting to create the buildid by explicitly running 'make export' in objdir/config: https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/scripts/mobile_l10n.py?from=mobile_l10n.py#370

I guess we'd need to change this to do 'make buildid.h' in objdir?
I think the buildid.h thing is kind of red herring, though. That is, it looks to me those builds ought to be setting MOZ_APP_ANDROID_VERSION_CODE, deriving it from the unpacked apk.
glandium, nalexander, and I discussed this in person and we concluded that we do still need to generate the buildid.h file here. This change removes the somewhat confusing 'make_dirs' configuration option, which is just ['config'] for all android variants anyway. We still need to run 'make export' in config in order to get nsinstall, and then we also need to run 'make buildid.h' in the top of the objdir to get the new buildid.h file.
Assignee: nobody → mshal
Attachment #8723665 - Flags: review?(jlund)
FWIW, I've tested this as best I can in staging, though I'm unable to do a release android l10n build correctly due to some crazyness with how mozharness downloads buildbot-configs.
Comment on attachment 8723665 [details] [diff] [review]
0001-Bug-1249912-Fix-the-buildid.h-generation-in-mobile_l.patch

Review of attachment 8723665 [details] [diff] [review]:
-----------------------------------------------------------------

mozharness code looks good. just some questions to get me up to speed.

::: testing/mozharness/scripts/mobile_l10n.py
@@ +367,5 @@
>                                error_list=MakefileErrorList):
>              self.fatal("Configure failed!")
> +
> +        # Run 'make export' in objdir/config to get nsinstall
> +        self.run_command_m([make, 'export'],

one of the changes in this patch is that we will no longer run `make export` with MOZ_BUILD_DATE passed. was that not needed in the first place? Is buildid.h replacing the original second export call?

@@ +376,5 @@
> +
> +        # Run 'make buildid.h' in objdir/ to get the buildid.h file
> +        cmd = [make, 'buildid.h']
> +        if buildid:
> +            cmd.append('MOZ_BUILD_DATE=%s' % str(buildid))

what happens in `make buildid.h` if buildid is not passed?
ni: comment 6 ^
Flags: needinfo?(mshal)
(In reply to Jordan Lund (:jlund) from comment #6)
> ::: testing/mozharness/scripts/mobile_l10n.py
> @@ +367,5 @@
> >                                error_list=MakefileErrorList):
> >              self.fatal("Configure failed!")
> > +
> > +        # Run 'make export' in objdir/config to get nsinstall
> > +        self.run_command_m([make, 'export'],
> 
> one of the changes in this patch is that we will no longer run `make export`
> with MOZ_BUILD_DATE passed. was that not needed in the first place? Is
> buildid.h replacing the original second export call?

Previously, we had a 'buildid' file generated by objdir/config during 'make export'. So MOZ_BUILD_DATE could be passed in to this make command to specify what buildid to use. However, that was changed in http://hg.mozilla.org/mozilla-central/rev/9f7915738557. Now, we have a 'buildid.h' file with the same information (though in a header-style format), and it's generated at the top-level:

http://hg.mozilla.org/mozilla-central/diff/02840dfc6310/moz.build#l1.20
http://hg.mozilla.org/mozilla-central/diff/02840dfc6310/build/variables.py#l1.17

So objdir/config/ no longer needs MOZ_BUILD_DATE for anything, and objdir/ can use MOZ_BUILD_DATE in the environment to force a specific buildid, similar to what we had before.

> @@ +376,5 @@
> > +
> > +        # Run 'make buildid.h' in objdir/ to get the buildid.h file
> > +        cmd = [make, 'buildid.h']
> > +        if buildid:
> > +            cmd.append('MOZ_BUILD_DATE=%s' % str(buildid))
> 
> what happens in `make buildid.h` if buildid is not passed?

It'll just generate a buildid using datetime.now:

http://hg.mozilla.org/mozilla-central/diff/02840dfc6310/build/variables.py#l1.22

which is the same as what we used to do with the Makefile.in & make-platformini.py:

http://hg.mozilla.org/mozilla-central/rev/9f7915738557#l3.22
http://hg.mozilla.org/mozilla-central/diff/02840dfc6310/toolkit/xre/make-platformini.py#l1.31
Flags: needinfo?(mshal)
Comment on attachment 8723665 [details] [diff] [review]
0001-Bug-1249912-Fix-the-buildid.h-generation-in-mobile_l.patch

Review of attachment 8723665 [details] [diff] [review]:
-----------------------------------------------------------------

thanks for the context. r+ ! :)
Attachment #8723665 - Flags: review?(jlund) → review+
Do we havesomething to make nsinstall is there? That used to be/is a side-effect of export in /config.
(In reply to Axel Hecht [:Pike] from comment #10)
> Do we havesomething to make nsinstall is there? That used to be/is a
> side-effect of export in /config.

Yes, I just separated that out so it is no longer a weird make_dirs variable in the mozharness configs. The patch has this for nsinstall:

+        # Run 'make export' in objdir/config to get nsinstall
+        self.run_command_m([make, 'export'],
+                           cwd=os.path.join(dirs['abs_objdir'], 'config'),
...

And this for the new buildid.h file:

+        # Run 'make buildid.h' in objdir/ to get the buildid.h file
+        cmd = [make, 'buildid.h']
+        if buildid:
+            cmd.append('MOZ_BUILD_DATE=%s' % str(buildid))
+        self.run_command_m(cmd,
+                           cwd=dirs['abs_objdir'],
...
https://hg.mozilla.org/mozilla-central/rev/5f8c7166c926
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 47 → mozilla47
You need to log in before you can comment on or make changes to this bug.