In the Makefile.in world, we support a _FLAGS argument to PP_TARGETS to provide additional things to the preprocessor, but in moz.build there is no equivalent. We should add support for that so we can pass in custom -D flags without affecting global defines, or flags like --marker, -F, etc.

mobile/android/base wants flags (--marker), custom defines (including ANDROID_VERSION_CODE produced by a Python script), and to preprocess one file into two destinations with a different DEFINE each time.

It turns out this is kinda tricky. Ideally I'd like to add a .flags attribute similar to what we have for SOURCES, but SOURCES is a ContextDerivedTypedListWithItems whereas FINAL_TARGET_PP_FILES and the like are ContextDerivedHierarchicalStringLists. The hierarchical part means we're assuming attributes are part of the hierarchy, so FINAL_TARGET_PP_FILES['foo.in'].flags += ['-Dblah'] would actually try to preprocess the file '-Dblah' and put it in dist/bin/foo.in/flags/, which is obviously not what we want. We could probably put in special logic to handle "flags", but I think that would be too obtuse. One thing that seems to work ok is to create a new variable, like PP_FLAGS, and then use the PerSourceFlag object to write out _FLAGS variables in the make backend. This is slightly different to how the recursive make backend handles PP_TARGETS currently, since the _FLAGS variable is per-grouping rather than per-file. However, it's easy enough to add $($(<F)_FLAGS) to the preprocessor command to pick those up. The upcoming patches go this route, though I'm not totally sold that this is the way to go. Another option might be to add both the files and the flags when adding to FINAL_TARGET_PP_FILES. Something like: FINAL_TARGET_PP_FILES += { 'files': ['foo.in'], 'flags': ['-DFOO=1', '--marker=//'], } Though we'd still want to handle the normal case of adding just files by appending a list, I think. So we'd have to have some special handling of allowing either a dict or a list. I'm not sure what would be involved to get that working within the hierarchical string list. I'm certainly open to other suggestions if anyone has one!

Review commit: https://reviewboard.mozilla.org/r/45889/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45889/

Review commit: https://reviewboard.mozilla.org/r/45891/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45891/

Review commit: https://reviewboard.mozilla.org/r/45893/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45893/

Review commit: https://reviewboard.mozilla.org/r/45895/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45895/

(In reply to Nick Alexander :nalexander from comment #1) > mobile/android/base wants flags (--marker), custom defines (including > ANDROID_VERSION_CODE produced by a Python script), and to preprocess one > file into two destinations with a different DEFINE each time. Hmm, I just noticed the "two destinations with a different DEFINE" part. Where does that happen? That won't work with the patches I just posted, but I don't see where it's used in the existing android Makefiles.

(In reply to Michael Shal [:mshal] from comment #7) > (In reply to Nick Alexander :nalexander from comment #1) > > mobile/android/base wants flags (--marker), custom defines (including > > ANDROID_VERSION_CODE produced by a Python script), and to preprocess one > > file into two destinations with a different DEFINE each time. > > Hmm, I just noticed the "two destinations with a different DEFINE" part. > Where does that happen? That won't work with the patches I just posted, but > I don't see where it's used in the existing android Makefiles. It's not used 'cuz it's too hard to arrange, leading to Bug 1249421. We want to replace a part of AndroidManifest.xml.in for Gradle's consumption, and my work-around (to do it in Gradle) isn't early enough: Android Studio looks for AndroidManifest.xml during import, which is before the Gradle code gets to run.

This is where, I think, contexts would be good to use. with FINAL_TARGET_PP_FILES['foo']: DEFINES['FOO'] = 'BAR' But the backends are not ready for these kind of things yet.

So, looking at the patches, I'm not very thrilled. The first thing that jumps to the eye, is that 2 of the patches are using your new PP_FLAGS for no real benefit. The second patch replaces benign DEFINES with PP_FLAGS. The fourth patch moves something from Makefile.in to use PP_FLAGS but could just as much use DEFINES instead. Which then leaves the third patch as the sole user of the feature, and it uses it to set --marker. So, the interesting thing about --marker, is that we already have a special case where we use a different marker than the default: css. And they're handled by file extension. No css file will be using # as a marker. And there is no reason for a java file to ever use # as a marker either, so we could just go with a per-extension thing for those files too.

(In reply to Mike Hommey [:glandium] from comment #10) > The first thing that jumps to the eye, is that 2 of the patches are using > your new PP_FLAGS for no real benefit. The second patch replaces benign > DEFINES with PP_FLAGS. The fourth patch moves something from Makefile.in to > use PP_FLAGS but could just as much use DEFINES instead. > Which then leaves the third patch as the sole user of the feature, and it > uses it to set --marker. This was a followup from: https://bugzilla.mozilla.org/show_bug.cgi?id=1252931#c27 Specifically, to get some of the PP_TARGETS into moz.build, the PP_FLAGS were moved into the per-directory DEFINES. This bug was meant to return to the original state of limiting the -Dtopsrcdir flags to just the PP_TARGETS that need them by adding support for that to moz.build. Personally I don't mind one way or the other. > So, the interesting thing about --marker, is that we already have a special > case where we use a different marker than the default: css. And they're > handled by file extension. No css file will be using # as a marker. And > there is no reason for a java file to ever use # as a marker either, so we > could just go with a per-extension thing for those files too. That's a good idea. It looks like this is just the one Android Makefile though, so I'll leave it for now.

(In reply to Mike Hommey [:glandium] from comment #9) > This is where, I think, contexts would be good to use. > > with FINAL_TARGET_PP_FILES['foo']: > DEFINES['FOO'] = 'BAR' > > But the backends are not ready for these kind of things yet. I like this approach a lot. Is there a bug already for it?

(In reply to Michael Shal [:mshal] from comment #12) > (In reply to Mike Hommey [:glandium] from comment #9) > > This is where, I think, contexts would be good to use. > > > > with FINAL_TARGET_PP_FILES['foo']: > > DEFINES['FOO'] = 'BAR' > > > > But the backends are not ready for these kind of things yet. > > I like this approach a lot. Is there a bug already for it? Not that I'm aware of.

I had a go at this one. I'm not wedded to any of these changes, particularly not to name choices. Some questions and explanations about what I changed: As mentioned in comment 2, using the item syntax for looking up files clashes with the use of the item syntax for specifying arbitrary directory names (eg. ones with periods in them). I forbade strings and children from having the same name and prioritized string lookup over children lookup and it works, but something like FINAL_TARGET_PP_FILES['foo@bar.com'].flags is ambiguous. By itself it refers to the directory srcdir/foo@bar.com/flags. With a string list specifying foo@bar.com as a file before it FINAL_TARGET_PP_FILES += ['foo@bar.com'] FINAL_TARGET_PP_FILES['foo@bar.com'].flags = '-Fsubstitution' it refers to the flags to be passed when preprocessing the file foo@bar.com. I'm not sure if this is too confusing/nonintuitive or what a preferred syntax would be. It felt like ensuring the lists in moz.build files are sorted was the important thing, rather than how they are stored in the backend (https://groups.google.com/d/topic/mozilla.dev.platform/f3CCQI1M2kQ/discussion), so I reordered the Mixin classes on ContextDerivedTypedHierarchicalStringList so the sorted check came before type conversion. This gives more flexibility for choosing the strings' storage class (it doesn't need to have a meaningful ordering defined). I changed the StringListAdaptor for generality because I couldn't find anything using the index lookup and I'd rather not add compatability code to MozlistDict for a feature that isn't being used.

Ok, after taking a step back, I realized prohibiting strings and children from having the same name won't work because it is possible someone would want to install a file in a directory of the same name as the source file. Will look into it further, hopefully this can be a starting point.

I changed lookup to a class method. FINAL_TARGET_PP_FILES.entry('foo@bar.com') Simple, efficient, unambiguous, ship it! Yes? Would allowing tuples or maybe dicts for setting preprocessor files be ok? FINAL_TARGET_PP_FILES += [ ('foo', '-Ffooflags'), {'file': 'bar', 'flags': '-Fbarflags'}, ]