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

Status: RESOLVED FIXED

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

Description

[Axel Hecht [:Pike]]

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?

Comment 1

[Nick Alexander :nalexander [he/him]]

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!

Comment 2

[Michael Shal [:mshal]]

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?

Comment 3

[Mike Hommey [:glandium]]

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.

Comment 4

[Michael Shal [:mshal]]

Attachment: 0001-Bug-1249912-Fix-the-buildid.h-generation-in-mobile_l.patch

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.

Comment 5

[Michael Shal [:mshal]]

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 6

[Jordan Lund (:jlund)]

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?

Comment 7

[Jordan Lund (:jlund)]

ni: comment 6 ^

Comment 8

[Michael Shal [: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

Comment 9

[Jordan Lund (:jlund)]

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+ ! :)

Comment 10

[Axel Hecht [:Pike]]

Do we havesomething to make nsinstall is there? That used to be/is a side-effect of export in /config.

Comment 11

[Michael Shal [:mshal]]

(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'],
...

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f8c7166c926

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5f8c7166c926