933300 - Generate preprocessed Java code into generated/org/mozilla/*.

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

The idea is that $OBJDIR/generated will have the Java package structure on disk, so that IDEs can link directly to it and the build system can manage the preprocessed input dependencies.  This is a big win for Bug 853045, which can get out of the "preprocessing Java" game.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: 933300.v1.patch

This is the least bad Makefile.in expression of the generation I could come up with.  There are a few things to note here, including why I didn't use PP_TARGETS:

1) PP_JAVAFILES is built from variables defined in moz.build.  That means it isn't available until after backend.mk is included from rules.mk, which means it can't be used as part of a PP_TARGETS declaration.  (At least, not without delicate = versus := checking.)

2) PP_TARGETS doesn't handle directories in the way we need.  Some of the preprocessed files live in m/a/b/SUBDIR and need to land in generated/org/mozilla/gecko/SUBDIR.  Automatically defining lots of different PP_TARGETS for these (in make syntax) is a pain.  I wish Makefiles had dicts!

3) It's not possible to make the input source file directory tree line up with the output directory tree since the output tree depends on ANDROID_PACKAGE_NAME.

4) moz.build syntax for generating files can't come soon enough!

Comment 2

[Nick Alexander :nalexander [he/him]]

This achieves some of the same ends as Bug 929865, but does so in a way that positions us to support IDEs in the future.  (We should still do Bug 929865, since that will alleviate some bad technical debt, but this should land independently, and probably before that bug.)

Comment 3

[Nick Alexander :nalexander [he/him]]

Try build with a slightly earlier version of this patch:

https://tbpl.mozilla.org/?tree=Try&rev=76c211f3c565

Comment 4

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 933300 - Part 2: Generate generated/org/mozilla/gecko/R.java.

Comment 5

[Nick Alexander :nalexander [he/him]]

Try build with both patches:

http://tbpl.mozilla.org/?tree=Try&rev=6b34688b0989

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 825339 [details] [diff] [review]
933300.v1.patch

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

::: mobile/android/base/Makefile.in
@@ +187,5 @@
> +# Certain source files need to be preprocessed.  This special rule
> +# generates these files into generated/org/mozilla/gecko for
> +# consumption by the build system and IDEs.
> +
> +preprocessed-tgts := $(foreach f,$(PP_JAVAFILES),$(if $(findstring $(gecko_package_dir),$(f)),$(f),))

Wouldn't $(filter $(gecko_package_dir)/%,$(PP_JAVAFILES)) do what you want or i'm missing something?

@@ +199,5 @@
> +# compatibility.  These special rules generate these files into
> +# generated/org/mozilla/{firefox,firefox_beta,fennec,fennec_$USER} for
> +# consumption by the build system and IDEs.
> +
> +preprocessed_package-tgts := $(foreach f,$(PP_JAVAFILES),$(if $(findstring $(android_package_dir),$(f)),$(f),))

Likewise.

@@ +209,5 @@
> +
> +define preprocess_template
> +$(1): $(2) $(3) $$(call mkdir_deps,$$(dir $(1))) $$(GLOBAL_DEPS)
> +	@$$(PYTHON) $$(topsrcdir)/config/Preprocessor.py \
> +             $$(AUTOMATION_PPARGS) $$(DEFINES) $$(ACDEFINES) $$< > $$@

I don't know where this AUTOMATION_PPARGS comes from originally, but it's useless. That being said, I'd prefer to add features to PP_TARGETS rather than having one-offs like this, which have their own glitches (like using ">", not quoting $< and $@, and not removing $@ first).

Also, I was about to file a bug to have dependencies handled by Preprocessor.py, because I spotted a few places were such dependencies are missing, and it turns out it's a problem here too. (And again, having a one-off here would mean fixing the one-off instead of fixing PP_TARGETS only ; arguably, we have many one-offs, but let's not add more)

I'm offering to implement the PP_TARGETS required changes.

::: mobile/android/base/android-services.mozbuild
@@ +789,5 @@
>  sync_generated_java_files = [
> +    'generated/org/mozilla/gecko/background/common/GlobalConstants.java',
> +    'generated/org/mozilla/gecko/sync/SyncConstants.java',
> +    'generated/org/mozilla/gecko/background/announcements/AnnouncementsConstants.java',
> +    'generated/org/mozilla/gecko/background/healthreport/HealthReportConstants.java',

The "generated" base directory is kind of a backend implementation detail. I don't think this should spill in moz.build definitions.

::: mobile/android/base/moz.build
@@ +321,4 @@
>      'R.java',
> +    'generated/' + android_package_dir + '/App.java',
> +    'generated/' + android_package_dir + '/WebApp.java',
> +    'generated/' + android_package_dir + '/WebApps.java',

For the future, it would be nice that generated_sources is a StrictOrderingOnAppendList. But since android_package_dir happens to be org.mozilla.fennec* or firefox*, it's not going to be a problem, but it would for anyone doing a rebranding (and mozilla china is doing that, although i don't remember what their ANDROID_PACKAGE_NAME is). Anyways, all that to say these three would be better added separately (and while at it, with a list generator)

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 825351 [details] [diff] [review]
Bug 933300 - Part 2: Generate generated/org/mozilla/gecko/R.java.

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

::: mobile/android/base/Makefile.in
@@ +80,5 @@
>    AndroidManifest.xml  \
>    classes.dex  \
>    gecko.ap_  \
>    res/values/strings.xml \
> +  generated/org/mozilla/gecko/R.java \

generated is in GARBAGE_DIRS. Why add this too?

@@ +271,5 @@
>  
> +.aapt.deps: $(all_resources)
> +	$(AAPT) package -f -M AndroidManifest.xml -I $(ANDROID_SDK)/android.jar -S res --custom-package org.mozilla.gecko --non-constant-id \
> +		-J generated/org/mozilla/gecko/ \
> +		-F gecko.ap_

Somehow I thought we already did that.

Comment 8

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #6)
> Comment on attachment 825339 [details] [diff] [review]
> 933300.v1.patch
> 
> Review of attachment 825339 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/Makefile.in
> @@ +187,5 @@
> > +# Certain source files need to be preprocessed.  This special rule
> > +# generates these files into generated/org/mozilla/gecko for
> > +# consumption by the build system and IDEs.
> > +
> > +preprocessed-tgts := $(foreach f,$(PP_JAVAFILES),$(if $(findstring $(gecko_package_dir),$(f)),$(f),))
> 
> Wouldn't $(filter $(gecko_package_dir)/%,$(PP_JAVAFILES)) do what you want
> or i'm missing something?

% interacts badly with paths, and I think an earlier version of this matched on a string in the middle (which failed).  I'll see if your version works.  (But I think it's really fragile!  I think it will match $(blah)/foo but not $(blah)/foo/bar, due to the paths.)

> @@ +209,5 @@
> > +
> > +define preprocess_template
> > +$(1): $(2) $(3) $$(call mkdir_deps,$$(dir $(1))) $$(GLOBAL_DEPS)
> > +	@$$(PYTHON) $$(topsrcdir)/config/Preprocessor.py \
> > +             $$(AUTOMATION_PPARGS) $$(DEFINES) $$(ACDEFINES) $$< > $$@
> 
> I don't know where this AUTOMATION_PPARGS comes from originally, but it's
> useless. That being said, I'd prefer to add features to PP_TARGETS rather
> than having one-offs like this, which have their own glitches (like using
> ">", not quoting $< and $@, and not removing $@ first).

Oh, I know, it was cargo-culted in.  I killed it but replaced it to reduce patch churn.

> Also, I was about to file a bug to have dependencies handled by
> Preprocessor.py, because I spotted a few places were such dependencies are
> missing, and it turns out it's a problem here too. (And again, having a
> one-off here would mean fixing the one-off instead of fixing PP_TARGETS only
> ; arguably, we have many one-offs, but let's not add more)
> 
> I'm offering to implement the PP_TARGETS required changes.

This is not the major issue.  The issue is that PP_TARGETS += target with target_FILES += foo/bar.in and baz/nuts.in doesn't write foo/bar and baz/nuts, it writes bar and nuts to the same directory.  This sucks but I didn't want to try to make sure every usage of PP_TARGETS doesn't assume this behaviour.  Thoughts?
 
> ::: mobile/android/base/android-services.mozbuild
> @@ +789,5 @@
> >  sync_generated_java_files = [
> > +    'generated/org/mozilla/gecko/background/common/GlobalConstants.java',
> > +    'generated/org/mozilla/gecko/sync/SyncConstants.java',
> > +    'generated/org/mozilla/gecko/background/announcements/AnnouncementsConstants.java',
> > +    'generated/org/mozilla/gecko/background/healthreport/HealthReportConstants.java',
> 
> The "generated" base directory is kind of a backend implementation detail. I
> don't think this should spill in moz.build definitions.

Duly noted.

> ::: mobile/android/base/moz.build
> @@ +321,4 @@
> >      'R.java',
> > +    'generated/' + android_package_dir + '/App.java',
> > +    'generated/' + android_package_dir + '/WebApp.java',
> > +    'generated/' + android_package_dir + '/WebApps.java',
> 
> For the future, it would be nice that generated_sources is a
> StrictOrderingOnAppendList. But since android_package_dir happens to be
> org.mozilla.fennec* or firefox*, it's not going to be a problem, but it
> would for anyone doing a rebranding (and mozilla china is doing that,
> although i don't remember what their ANDROID_PACKAGE_NAME is). Anyways, all
> that to say these three would be better added separately (and while at it,
> with a list generator)

Yeah, in one iteration I actually split that append for precisely this reason.  I'll rev the type of sources and generated_sources.

Comment 9

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #7)
> Comment on attachment 825351 [details] [diff] [review]
> Bug 933300 - Part 2: Generate generated/org/mozilla/gecko/R.java.
> 
> Review of attachment 825351 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/Makefile.in
> @@ +80,5 @@
> >    AndroidManifest.xml  \
> >    classes.dex  \
> >    gecko.ap_  \
> >    res/values/strings.xml \
> > +  generated/org/mozilla/gecko/R.java \
> 
> generated is in GARBAGE_DIRS. Why add this too?

'cuz all my examples include both.  Happy to just do GARBAGE_DIRS.

> @@ +271,5 @@
> >  
> > +.aapt.deps: $(all_resources)
> > +	$(AAPT) package -f -M AndroidManifest.xml -I $(ANDROID_SDK)/android.jar -S res --custom-package org.mozilla.gecko --non-constant-id \
> > +		-J generated/org/mozilla/gecko/ \
> > +		-F gecko.ap_
> 
> Somehow I thought we already did that.

We did it for the generic rules (used for all other APKs).  I have WIP toward building other APKs using moz.build, at which point I'll move mobile/android/base's APK over to the generic rules, but for now, this.

Comment 10

[Mike Hommey [:glandium]]

(In reply to Nick Alexander :nalexander from comment #8)
> This is not the major issue.  The issue is that PP_TARGETS += target with
> target_FILES += foo/bar.in and baz/nuts.in doesn't write foo/bar and
> baz/nuts, it writes bar and nuts to the same directory.  This sucks but I
> didn't want to try to make sure every usage of PP_TARGETS doesn't assume
> this behaviour.  Thoughts?

That's why I said I'd prefer to add features to PP_TARGETS. Preserving directories is common enough in other uses of the preprocessor to grant it being added.

Comment 11

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #10)
> (In reply to Nick Alexander :nalexander from comment #8)
> > This is not the major issue.  The issue is that PP_TARGETS += target with
> > target_FILES += foo/bar.in and baz/nuts.in doesn't write foo/bar and
> > baz/nuts, it writes bar and nuts to the same directory.  This sucks but I
> > didn't want to try to make sure every usage of PP_TARGETS doesn't assume
> > this behaviour.  Thoughts?
> 
> That's why I said I'd prefer to add features to PP_TARGETS. Preserving
> directories is common enough in other uses of the preprocessor to grant it
> being added.

Filed Bug 934864.

Comment 12

[Nick Alexander :nalexander [he/him]]

Attachment: Review comments.

* Don't require generated/ in moz.build: this isolates an
  implementation detail.
* Use PP_TARGETS with KEEP_PATH.
* Make javac_flags a list; order source lists.

Comment 13

[Nick Alexander :nalexander [he/him]]

https://tbpl.mozilla.org/?tree=Try&rev=fe3519c96513

Comment 14

[Nick Alexander :nalexander [he/him]]

http://tbpl.mozilla.org/?tree=Try&rev=2df93df8a97d

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 828300 [details] [diff] [review]
Review comments.

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

::: mobile/android/base/Makefile.in
@@ +1,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # 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/.
>  
> +include $(topsrcdir)/config/config.mk

You could place this where rules.mk was.

@@ +163,1 @@
>  # be defined after rules.mk is included.

after config.mk is included.

@@ +173,5 @@
>    WebAppFragmentRepeater.inc \
>    $(wildcard $(topsrcdir)/mobile/android/services/manifests/*.in) \
>    $(NULL)
>  
> +$(android): $(android-preqs)

You don't need these preqs anymore. (the dependency was wrong anyways)

@@ +181,5 @@
>  # generates these files into generated/org/mozilla/gecko for
>  # consumption by the build system and IDEs.
>  
> +# We would prefer to use $(filter, $(gecko_package_dir)/%, ...) but it
> +# doesn't work because % matches paths specially.

I don't think that's true.

@@ +191,5 @@
>  
> +preprocessed_PATH := $(gecko_package_dir)
> +preprocessed_KEEP_PATH := 1
> +
> +$(preprocessed): $(preprocessed-preqs)

Same as above, this is unneeded, and wrong anyways.

@@ +215,2 @@
>  
> +$(preprocessed_package): $(preprocessed_package-preqs)

Likewise.

::: python/mozbuild/mozbuild/frontend/data.py
@@ +20,5 @@
>  import os
>  
>  from collections import OrderedDict
> +from mozbuild.util import (
> +    StrictOrderingOnAppendList,

Put that on one line?

@@ +444,1 @@
>          self.extra_jars = list(extra_jars)

Why not this one too?

Comment 16

[Nick Alexander :nalexander [he/him]]

> @@ +173,5 @@
> >    WebAppFragmentRepeater.inc \
> >    $(wildcard $(topsrcdir)/mobile/android/services/manifests/*.in) \
> >    $(NULL)
> >  
> > +$(android): $(android-preqs)
> 
> You don't need these preqs anymore. (the dependency was wrong anyways)

Why not?  I rewrote these preqs after hand checking what AndroidManifest.xml.in includes.  Is this automatic now?  Bug number?

> @@ +181,5 @@
> >  # generates these files into generated/org/mozilla/gecko for
> >  # consumption by the build system and IDEs.
> >  
> > +# We would prefer to use $(filter, $(gecko_package_dir)/%, ...) but it
> > +# doesn't work because % matches paths specially.
> 
> I don't think that's true.

I can guarantee that this doesn't work.  I remember it matching org/mozilla/gecko/foo but not org/mozilla/gecko/foo/bar.  If you have a different explanatory comment, I'll include it, but this is the explanation that I've arrived at.

> @@ +191,5 @@
> >  
> > +preprocessed_PATH := $(gecko_package_dir)
> > +preprocessed_KEEP_PATH := 1
> > +
> > +$(preprocessed): $(preprocessed-preqs)
> 
> Same as above, this is unneeded, and wrong anyways.
> 
> @@ +215,2 @@
> >  
> > +$(preprocessed_package): $(preprocessed_package-preqs)
> 
> Likewise.

I must be missing something.  How are these preqs defined unless I do it by hand?  Is my syntax wrong?

> ::: python/mozbuild/mozbuild/frontend/data.py
> @@ +20,5 @@
> >  import os
> >  
> >  from collections import OrderedDict
> > +from mozbuild.util import (
> > +    StrictOrderingOnAppendList,
> 
> Put that on one line?
> 
> @@ +444,1 @@
> >          self.extra_jars = list(extra_jars)
> 
> Why not this one too?

A quick Google confirms that JAR ordering does matter.  It's not an issue for our codebase.  I see no reason to make this ordered but if you care, let's do it.

Comment 17

[Mike Hommey [:glandium]]

(In reply to Nick Alexander :nalexander from comment #16)
> > @@ +173,5 @@
> > >    WebAppFragmentRepeater.inc \
> > >    $(wildcard $(topsrcdir)/mobile/android/services/manifests/*.in) \
> > >    $(NULL)
> > >  
> > > +$(android): $(android-preqs)
> > 
> > You don't need these preqs anymore. (the dependency was wrong anyways)
> 
> Why not?  I rewrote these preqs after hand checking what
> AndroidManifest.xml.in includes.

You're writing that creating AndroidManifest.xml.in requires $(android-preqs), not that creating AndroidManifest.xml from it does. Likewise for the others.

> Is this automatic now?  Bug number?

Yes, since bug 935305.

> > @@ +181,5 @@
> > >  # generates these files into generated/org/mozilla/gecko for
> > >  # consumption by the build system and IDEs.
> > >  
> > > +# We would prefer to use $(filter, $(gecko_package_dir)/%, ...) but it
> > > +# doesn't work because % matches paths specially.
> > 
> > I don't think that's true.
> 
> I can guarantee that this doesn't work.  I remember it matching
> org/mozilla/gecko/foo but not org/mozilla/gecko/foo/bar.  If you have a
> different explanatory comment, I'll include it, but this is the explanation
> that I've arrived at.

$ cat > test.mk <<EOF
foo = a/b/c/d
$(warning $(filter a/b/%,$(foo)))
EOF
$ make -f test.mk
test.mk:2: a/b/c/d

It matches whether foo is a/b/c, a/b/c/d or anything else that starts with "a/b/". Obviously, that doesn't include "a/b", but if you need that, you can do $(filter a/b a/b/%,$(foo)))
 
> A quick Google confirms that JAR ordering does matter.  It's not an issue
> for our codebase.  I see no reason to make this ordered but if you care,
> let's do it.

If ordering matters, then StrictOrderingOnAppendList doesn't make sense here.

Comment 18

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #17)
> (In reply to Nick Alexander :nalexander from comment #16)
> > > @@ +173,5 @@
> > > >    WebAppFragmentRepeater.inc \
> > > >    $(wildcard $(topsrcdir)/mobile/android/services/manifests/*.in) \
> > > >    $(NULL)
> > > >  
> > > > +$(android): $(android-preqs)
> > > 
> > > You don't need these preqs anymore. (the dependency was wrong anyways)
> > 
> > Why not?  I rewrote these preqs after hand checking what
> > AndroidManifest.xml.in includes.
> 
> You're writing that creating AndroidManifest.xml.in requires
> $(android-preqs), not that creating AndroidManifest.xml from it does.
> Likewise for the others.

Thanks.

> > Is this automatic now?  Bug number?
> 
> Yes, since bug 935305.

This is awesome!  Thanks for implementing this.

Comment 19

[Nick Alexander :nalexander [he/him]]

Attachment: Review comments 2.

Comment 20

[Nick Alexander :nalexander [he/him]]

Comment on attachment 830624 [details] [diff] [review]
Review comments 2.

Bug 953305 really made this better.  Thanks!

Comment 21

[Mike Hommey [:glandium]]

Comment on attachment 830624 [details] [diff] [review]
Review comments 2.

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

This makes this file a little bit nicer.

Comment 22

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/b2g-inbound/rev/ba9c8fcf76e6

Comment 23

[Nick Alexander :nalexander [he/him]]

Pretty sure this will burn a clobber build, so backing out to be safe:

https://hg.mozilla.org/integration/b2g-inbound/rev/779cd8e878d7

Comment 24

[Nick Alexander :nalexander [he/him]]

Relanded without changes.  I got confused.

https://hg.mozilla.org/integration/b2g-inbound/rev/1a23bfa677c1

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a23bfa677c1