Closed Bug 1221453 Opened 4 years ago Closed 4 years ago

Use Path classes for LocalInclude/GeneratedIncludes

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox45 fixed)

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(3 files, 1 obsolete file)

No description provided.
Assignee: nobody → mh+mozilla
Attachment #8682964 - Flags: review?(gps)
The %%%s things are kind of horrible, but it's better than nothing at the moment. In the end, I'd like to be able to do something like '%{ANDROID_SOURCE}'.format(CONFIG) but that currently doesn't work because format uses the dict methods directly aiui, and CONFIG is not a real dict.
Attachment #8682966 - Flags: review?(gps)
FWIW, I'm still fighting with the last patch because a) TC is busted on try for me, so half b2g jobs are not running, and b) several include paths don't exist on all versions of the gonk tree, so there needs to be some more adjustments.
I'd still like a review on the principle anyways (the end result will just have a few more conditionals on ANDROID_VERSION and/or less include paths).
Such a mess...
Attachment #8682966 - Attachment is obsolete: true
Attachment #8682966 - Flags: review?(gps)
Attachment #8683587 - Flags: review?(gps)
Comment on attachment 8682964 [details] [diff] [review]
Use SourcePaths for LOCAL_INCLUDES

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +876,5 @@
> +    def _pretty_path(self, path, backend_file):
> +        assert isinstance(path, Path)
> +        if isinstance(path, SourcePath):
> +            if path.full_path.startswith(backend_file.srcdir):
> +                return '$(srcdir)' + path.full_path[len(backend_file.srcdir):]

mozpath.relpath?

@@ +1239,5 @@
>              self.backend_input_files |= obj.manifest.manifests
>  
>      def _process_local_include(self, local_include, backend_file):
> +        path = self._pretty_path(local_include, backend_file)
> +        if ' ' in path:

Why was this check added? Can't we always single quote the output?
Attachment #8682964 - Flags: review?(gps) → review+
Comment on attachment 8682965 [details] [diff] [review]
Use ObjDirPaths for GENERATED_INCLUDES and merge with LOCAL_INCLUDES

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

I love the Path class.

::: media/webrtc/signaling/test/common.build
@@ +14,2 @@
>  LOCAL_INCLUDES += [
> +  '!/dom/bindings',

This indentation looks wrong?
Attachment #8682965 - Flags: review?(gps) → review+
Comment on attachment 8683587 [details] [diff] [review]
Use AbsolutePaths with LOCAL_INCLUDES instead of manual -I in CXXFLAGS

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

::: dom/media/encoder/moz.build
@@ +44,5 @@
>  LOCAL_INCLUDES += ['/ipc/chromium/src']
> +
> +if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk':
> +    LOCAL_INCLUDES += [
> +        '%%%s/%s' % (CONFIG['ANDROID_SOURCE'], d) for d in [

The '%%%s' isn't very intuitive unless people know '%%' is escaping '%' in % formatting strings in Python :/

Do you think this would be better written as '%' + '%s/%s' % (...) ?

::: dom/media/omx/moz.build
@@ +110,2 @@
>          'frameworks/native/include',
> +        'frameworks/native/opengl/include',

The reordering here brings up something I missed in part 1: this list is being converted to a sorted list. I imagine there will be a scenario where we want to preserve ordering in include paths due to similar path names. Yes, you can defeat this by using multiple LOCAL_INCLUDES. But it feels weird.

I do think keeping things sorted out of principle is probably the right thing to do, even if it may result in multiple LOCAL_INCLUDES hacks later.
Attachment #8683587 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #6)
> Comment on attachment 8682964 [details] [diff] [review]
> Use SourcePaths for LOCAL_INCLUDES
> 
> Review of attachment 8682964 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mozbuild/mozbuild/backend/recursivemake.py
> @@ +876,5 @@
> > +    def _pretty_path(self, path, backend_file):
> > +        assert isinstance(path, Path)
> > +        if isinstance(path, SourcePath):
> > +            if path.full_path.startswith(backend_file.srcdir):
> > +                return '$(srcdir)' + path.full_path[len(backend_file.srcdir):]
> 
> mozpath.relpath?

As mentioned on irc, my rationale for not using relpath is that it's slower and we

> 
> @@ +1239,5 @@
> >              self.backend_input_files |= obj.manifest.manifests
> >  
> >      def _process_local_include(self, local_include, backend_file):
> > +        path = self._pretty_path(local_include, backend_file)
> > +        if ' ' in path:
> 
> Why was this check added? Can't we always single quote the output?

We can, but I don't really like quoting everything when it can be avoided. It doesn't help command lines readability.
Blocks: 1222472
Blocks: 1222479
I'm encountering strange problems with header files not being found e.g.

c:/t1/hg/comm-central/mailnews/base/util/nsMsgUtils.cpp(52) : fatal error C1083: Cannot open include file: 'nsProtocolProxyService.h': No such file or directory

c:\t1\hg\objdir-sm\dist\include\CertVerifier.h(11) : fatal error C1083: Cannot open include file: 'pkix/pkixtypes.h': No such file or directory

Backing out the three patches in this bug allows my compile to succeed.
Flags: needinfo?(mh+mozilla)
> Backing out the three patches in this bug allows my compile to succeed.

That doesn't make sense considering bug 1222643 and bug 1222479. That is, if you don't also backout those at the same time, you should get config.status errors well before hitting anything else.

Also, please file bugs as bugs, not as comments in fixed bugs, and please include more output. Error messages alone are rarely enough.
Flags: needinfo?(mh+mozilla)
(In reply to Philip Chee from comment #12)
> I'm encountering strange problems with header files not being found e.g.
> 
> c:/t1/hg/comm-central/mailnews/base/util/nsMsgUtils.cpp(52) : fatal error
> C1083: Cannot open include file: 'nsProtocolProxyService.h': No such file or
> directory
> 
> c:\t1\hg\objdir-sm\dist\include\CertVerifier.h(11) : fatal error C1083:
> Cannot open include file: 'pkix/pkixtypes.h': No such file or directory
> 
> Backing out the three patches in this bug allows my compile to succeed.

The bug was noticed by aleth and is filed in
Bug 1222591 - nsMsgUtils.cpp:52:10: fatal error: 'nsProtocolProxyService.h' file not found
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.