1055281 - Make it an error to add a non-existent directory to LOCAL_INCLUDES

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(no longer active)]

Currently, it just works!

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: check for existence of paths in LOCAL_INCLUDES in moz.build

Like all easy patches, this one took a bit longer than necessary, tracking down
all the bogus includes people have added.  This patch builds on my Linux64
machine; I have no doubt that a |-p all| try build will show up a few more
problems.

I'm not a fan of the $(DEPTH) hack, but I'm not sure of an easy workaround.
Over in bug 976733, we were discussing having objdir paths in moz.build files
start with '!'.  That might be one way of fixing this; the translation could
then add $(DEPTH) as appropriate.  Followup bug, WDYT?

Comment 2

[Michael Shal [:mshal]]

Comment on attachment 8475931 [details] [diff] [review]
check for existence of paths in LOCAL_INCLUDES in moz.build

> if CONFIG['ENABLE_TESTS']:
>     LOCAL_INCLUDES += [
>-      'ipc/glue',
>+      '/ipc/glue',
>     ]

If things compiled with the previous (non-existent) path, presumably it isn't necessary at all?

> if CONFIG['MOZ_FFMPEG']:
>     LOCAL_INCLUDES += [
>-        '/content/media/fmp4/ffmpeg/include',
>+        '/content/media/fmp4/ffmpeg',
>     ]

Similarly here.

>-        # Danger: this is to include config.h. This could be bad.
>-        '../trunk/third_party/libsrtp/config',

Hah, nice :). This proves we're less bad!

>-            yield FinalTargetFiles(sandbox, final_target_files, sandbox['FINAL_TARGET'])
>+            yield FinalTargetFiles(context, final_target_files, context['FINAL_TARGET'])

Is this supposed to be with another patch? I don't see what it has to do with LOCAL_INCLUDES. (Plus FINAL_TARGET_FILES isn't in the tree yet, it seems).

Comment 3

[Michael Shal [:mshal]]

(In reply to Nathan Froyd (:froydnj) from comment #1)
> I'm not a fan of the $(DEPTH) hack, but I'm not sure of an easy workaround.
> Over in bug 976733, we were discussing having objdir paths in moz.build files
> start with '!'.  That might be one way of fixing this; the translation could
> then add $(DEPTH) as appropriate.  Followup bug, WDYT?

I don't know if it really qualifies as a "hack" so much. Currently you essentially have "$(DEPTH)" as the path-is-relative-to-objdir token. If that token instead becomes "!", then the code changes from:

if local_include.startswith('$(DEPTH)'):

to:

if local_include.startswith('!'):

Right?

IOW - I don't think it's a hack because you need the logic to support objdir-relative paths anyway. Whether those paths should be denoted with $(DEPTH) or ! or something else, I don't personally have an opinion on :)

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Michael Shal [:mshal] from comment #2)
> >-            yield FinalTargetFiles(sandbox, final_target_files, sandbox['FINAL_TARGET'])
> >+            yield FinalTargetFiles(context, final_target_files, context['FINAL_TARGET'])
> 
> Is this supposed to be with another patch? I don't see what it has to do
> with LOCAL_INCLUDES. (Plus FINAL_TARGET_FILES isn't in the tree yet, it
> seems).

Yeah, this is what I get for trying to do development and fixing up rebasing at the same time.

Anyway, for those playing along at home, this is not straightforward as one might like, because gyp likes to add completely non-existent paths in various places.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: update gyp_reader to cope with new LOCAL_INCLUDES regime

Instead of doing consistency checking prior to reading GYP_DIRS, I thought I'd
clean up gyp's input prior to stuffing it into moz.build.  I realize the
duplication is not the best, but this way we get to validate all of gyp's
stuff, too.

I found a few more moz.build files that needed to be fixed up, all
android-specific.  I trust that you don't need to review those to approve them.

There'll be a test-only patch coming sometime tomorrow; as might be expected,
the LOCAL_INCLUDES tests all have non-existent directories and fall over. :(

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8475931 [details] [diff] [review]
check for existence of paths in LOCAL_INCLUDES in moz.build

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

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +580,5 @@
> +        for local_include in context.get('LOCAL_INCLUDES', []):
> +            # XXX hack for things built with gyp
> +            if local_include.startswith('$(DEPTH)'):
> +                yield LocalInclude(context, local_include)
> +                continue

gyp-specific hacks would be better in gyp_reader.py. Where does the DEPTH come from, btw?

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #6)
> Comment on attachment 8475931 [details] [diff] [review]
> check for existence of paths in LOCAL_INCLUDES in moz.build
> 
> Review of attachment 8475931 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mozbuild/mozbuild/frontend/emitter.py
> @@ +580,5 @@
> > +        for local_include in context.get('LOCAL_INCLUDES', []):
> > +            # XXX hack for things built with gyp
> > +            if local_include.startswith('$(DEPTH)'):
> > +                yield LocalInclude(context, local_include)
> > +                continue
> 
> gyp-specific hacks would be better in gyp_reader.py. Where does the DEPTH
> come from, btw?

It comes from here:

http://mxr.mozilla.org/mozilla-central/source/media/libyuv/libyuv.gyp#129

which is a) unnecessary given the gyp_reader.py checking we now do; and b) an unnecessary addition because presumably we already have $(topobjdir)/dist/include in our include path.  I'll remove the hack assuming the gyp_reader.py bit goes through.

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: fixup tests for stricter LOCAL_INCLUDES validation

This patch is just creating the directory structure that current LOCAL_INCLUDES
tests expect to exist, and adding a test for missing directory structure.  The
dummy_file_for_nonempty_directory bits are so git will accept the directories
as part of source control; I don't know whether mercurial has such limitations
or not...

I'll fold all these prior to commit.

Comment 9

[Michael Shal [:mshal]]

Comment on attachment 8476224 [details] [diff] [review]
update gyp_reader to cope with new LOCAL_INCLUDES regime

I'm a bit concerned that the full paths that aren't supposed to be topsrcdir-relative are silently dropped. I'm sure that will be confusing for someone who tries to add such a path to a gyp file in the future.

Additionally, since we know we're dropping the $(DEPTH)/dist/include path here, can we just remove that from libyuv.gyp as part of this bug? It was added by us, and I'd rather not have a path that looks like it should be there, but we silently drop elsewhere.

Comment 10

[(no longer active)]

(In reply to Nathan Froyd (:froydnj) from comment #1)
> Created attachment 8475931 [details] [diff] [review]
> check for existence of paths in LOCAL_INCLUDES in moz.build
> 
> Like all easy patches, this one took a bit longer than necessary, tracking
> down
> all the bogus includes people have added.  This patch builds on my Linux64
> machine; I have no doubt that a |-p all| try build will show up a few more
> problems.
> 
> I'm not a fan of the $(DEPTH) hack, but I'm not sure of an easy workaround.
> Over in bug 976733, we were discussing having objdir paths in moz.build files
> start with '!'.  That might be one way of fixing this; the translation could
> then add $(DEPTH) as appropriate.  Followup bug, WDYT?

Sorry, I didn't follow all of the discussion here, but isn't that what GENERATED_INCLUDES is supposed to do?

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #10)
> (In reply to Nathan Froyd (:froydnj) from comment #1)
> > Created attachment 8475931 [details] [diff] [review]
> > check for existence of paths in LOCAL_INCLUDES in moz.build
> > 
> > Like all easy patches, this one took a bit longer than necessary, tracking
> > down
> > all the bogus includes people have added.  This patch builds on my Linux64
> > machine; I have no doubt that a |-p all| try build will show up a few more
> > problems.
> > 
> > I'm not a fan of the $(DEPTH) hack, but I'm not sure of an easy workaround.
> > Over in bug 976733, we were discussing having objdir paths in moz.build files
> > start with '!'.  That might be one way of fixing this; the translation could
> > then add $(DEPTH) as appropriate.  Followup bug, WDYT?
> 
> Sorry, I didn't follow all of the discussion here, but isn't that what
> GENERATED_INCLUDES is supposed to do?

I don't follow this question at all.

(In any event, the $(DEPTH) was coming from a gyp file, and the include path was $(DEPTH)/dist/include, which was obviously unnecessary.)

Comment 12

[(no longer active)]

(In reply to Nathan Froyd (:froydnj) from comment #11)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #10)
> > (In reply to Nathan Froyd (:froydnj) from comment #1)
> > > Created attachment 8475931 [details] [diff] [review]
> > > check for existence of paths in LOCAL_INCLUDES in moz.build
> > > 
> > > Like all easy patches, this one took a bit longer than necessary, tracking
> > > down
> > > all the bogus includes people have added.  This patch builds on my Linux64
> > > machine; I have no doubt that a |-p all| try build will show up a few more
> > > problems.
> > > 
> > > I'm not a fan of the $(DEPTH) hack, but I'm not sure of an easy workaround.
> > > Over in bug 976733, we were discussing having objdir paths in moz.build files
> > > start with '!'.  That might be one way of fixing this; the translation could
> > > then add $(DEPTH) as appropriate.  Followup bug, WDYT?
> > 
> > Sorry, I didn't follow all of the discussion here, but isn't that what
> > GENERATED_INCLUDES is supposed to do?
> 
> I don't follow this question at all.
> 
> (In any event, the $(DEPTH) was coming from a gyp file, and the include path
> was $(DEPTH)/dist/include, which was obviously unnecessary.)

Hmm, it looks like I was confused, sorry.

Comment 13

[Nathan Froyd [:froydnj]]

This landed: http://hg.mozilla.org/mozilla-central/rev/b4356b4f14de

I guess mcMerge failed to pick it up.