Closed Bug 1428608 Opened 6 years ago Closed 5 years ago

Thunderbird buildbot builds busted with dom/canvas/WebGLContext.cpp(17): fatal error C1083: Cannot open include file: 'gfx/gl/MozFramebuffer.h': No such file or directory (working on TC builds)

Categories

(Thunderbird :: Build Config, defect, P1)

Product:
Thunderbird
Component:
Build Config
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 59.0

People

(Reporter: jorgk-bmo, Assigned: glandium)

References

Details

(Whiteboard: gfx-noted)

Attachments

(1 file, 4 obsolete files)

Bug 1428608 - Forbid / or !/ in LOCAL_INCLUDES.
59 bytes, text/x-review-board-request
froydnj
: review+
Details 
+++ This bug was initially created as a clone of Bug #1427668 +++

This follows on from bug 1427668 comment #64 to comment bug 1427668 comment #67.

TB buildbot builds fail with:
mozilla/dom/canvas/WebGLContext.cpp(17): fatal error C1083: Cannot open include file: 'gfx/gl/MozFramebuffer.h': No such file or directory

Tom said:
This appears to be due to https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/context.py#664 and https://hg.mozilla.org/mozilla-central/diff/e2b263515feb/dom/canvas/moz.build#l1.12 interacting poorly.
Severity: normal → blocker
status-firefox59: affected → ---
Component: Canvas: WebGL → Build Config
Product: Core → Thunderbird
Comment on attachment 8940581 [details]
Bug 1428608: Use EXTERNAL_SOURCE_DIRS to explicity refer to paths in comm-central, when building with c-c as topsrcdir;

https://reviewboard.mozilla.org/r/210778/#review216668

I didn't know EXTERNAL_SOURCE_DIRS did this, but you are the expert :)
I think eventually we should deprecate building with c-c as topsrcdir and make client.py migrate it for the developer.
Attachment #8940581 - Flags: review?(philipp) → review+
Comment on attachment 8940581 [details]
Bug 1428608: Use EXTERNAL_SOURCE_DIRS to explicity refer to paths in comm-central, when building with c-c as topsrcdir;

https://reviewboard.mozilla.org/r/210778/#review216668

I just introduced `EXTERNAL_SOURCE_DIRS` in the m-c patch series associated to this issue. :)

I agree that we should deprecate building with c-c as topsrcdir. I plan to remove support for doing that some time after 59 branches. We could start documenting and depracting support now though. I've got enough on my plate that I haven't started to think about documenting the new state of affairs, or automating the transition yet.
Comment on attachment 8940581 [details]
Bug 1428608: Use EXTERNAL_SOURCE_DIRS to explicity refer to paths in comm-central, when building with c-c as topsrcdir;

Thanks. SeaMonkey and extensions build.
Attachment #8940581 - Flags: review?(frgrahl) → review+
This is probably because I tried adding '.' to the local include path, so that '.' + 'gfx/gl/MozFramebuffer.h' should resolve without having to export gfx/gl/MozFramebuffer into our pseudo-namespace include dirs.
(In reply to Jeff Gilbert [:jgilbert] from comment #10)
> This is probably because I tried adding '.' to the local include path, so
> that '.' + 'gfx/gl/MozFramebuffer.h' should resolve without having to export
> gfx/gl/MozFramebuffer into our pseudo-namespace include dirs.

Yes, that plus the fact that a thunderbird build would interpret that as being the root of comm-central, rather than mozilla-central. There is a heuristic to treat paths as being in c-c if they exist there. This is only used in a couple of places, and in every case we know which tree we want to look for the file in. So, this changes the build system to not guess, and the files in C-C that were depending on that heuristic to be explicit that they want to look in c-c.
(In reply to Tom Prince [:tomprince] from comment #11)
> (In reply to Jeff Gilbert [:jgilbert] from comment #10)
> > This is probably because I tried adding '.' to the local include path, so
> > that '.' + 'gfx/gl/MozFramebuffer.h' should resolve without having to export
> > gfx/gl/MozFramebuffer into our pseudo-namespace include dirs.
> 
> Yes, that plus the fact that a thunderbird build would interpret that as
> being the root of comm-central, rather than mozilla-central. There is a
> heuristic to treat paths as being in c-c if they exist there. This is only
> used in a couple of places, and in every case we know which tree we want to
> look for the file in. So, this changes the build system to not guess, and
> the files in C-C that were depending on that heuristic to be explicit that
> they want to look in c-c.

Oh, I feared that might be the case! This sounds like a great change, though.
See Also: → 1428678
Comment on attachment 8940578 [details]
Bug 1428608: Avoid capturing mutable `DIRS` list from moz.build context;

https://reviewboard.mozilla.org/r/210782/#review216838

I think this makes sense, but I'd prefer you be explicit: `= list(...)` rather than `+=`, which looks like it's trying to extend the list rather than set it.
Attachment #8940578 - Flags: review+
Comment on attachment 8940579 [details]
Bug 1428608: Allow explicitly specifying dirs from EXTERNAL_SOURCE_DIR to recurse into;

https://reviewboard.mozilla.org/r/210784/#review216834

This is an r+ technically, 'cuz I think it does what you want.  But it's an r- to flag gps or glandium for a final review, since it's not clear we want this bleed through on the directory traversal scheme.  It's not clear to me why this is necessary, and if it's the best way to achieve the goal.

::: python/mozbuild/mozbuild/frontend/context.py:694
(Diff revision 1)
> +            raise ValueError('Path \'%s\' is not absolute' % value)
> +        # We explicitly skip calling SourcePath's __init__ here
> +        super(SourcePath, self).__init__(context, value)
> +
> +        if context.config.external_source_dir:
> +            path = mozpath.join(context.config.external_source_dir,

Please assert that `value` is non-empty and that `value[0]` is what you expect ('/')?  And that `value[1]` is not `/`.

::: python/mozbuild/mozbuild/frontend/context.py:710
(Diff revision 1)
> +
> +    @memoized_property
> +    def translated(self):
> +        """Returns the corresponding path in the objdir.
> +
> +        Ideally, we wouldn't need this function, but the fact that both source

This comment feels culted.  Is it correct?
Attachment #8940579 - Flags: review-
Comment on attachment 8940580 [details]
Bug 1428608: Don't guess that paths might be in EXTERNAL_SOURCE_DIR;

https://reviewboard.mozilla.org/r/210786/#review216840

If the approach and previous commit is acceptable to the most knowledgeable build peers, then this patch is fine too.[
Attachment #8940580 - Flags: review+
Attachment #8940578 - Flags: review?(core-build-config-reviews)
Attachment #8940579 - Flags: review?(core-build-config-reviews)
Attachment #8940580 - Flags: review?(core-build-config-reviews)
Comment on attachment 8940578 [details]
Bug 1428608: Avoid capturing mutable `DIRS` list from moz.build context;

https://reviewboard.mozilla.org/r/210782/#review216838

Well, it is extending the [default](https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/data.py#123). Would it be better to pass in the list, rather than mutating it?
Attachment #8940579 - Flags: review?(mh+mozilla)
Attachment #8940579 - Flags: review?(gps)
Comment on attachment 8940578 [details]
Bug 1428608: Avoid capturing mutable `DIRS` list from moz.build context;

https://reviewboard.mozilla.org/r/210782/#review216838

I agree that it's technically correct, it's just a little misleading and less definitive than what you were replacing.  I leave it to your judgement.
Comment on attachment 8940579 [details]
Bug 1428608: Allow explicitly specifying dirs from EXTERNAL_SOURCE_DIR to recurse into;

https://reviewboard.mozilla.org/r/210784/#review216834

`c-c/mail/app.mozbuild` and `c-c/suite/app.mozbuild` (and indirectly `c-c/mailnews/mailnews.mozbuild`) are included by the root `m-c/moz.build` ([here](https://dxr.mozilla.org/mozilla-central/source/moz.build#102)). They need to specify recursion to a mix of both [c-c](https://dxr.mozilla.org/comm-central/source/mail/app.mozbuild#27-31) and [m-c](https://dxr.mozilla.org/comm-central/source/mail/app.mozbuild#16). Currently, this is handled by a heuristic (removed in the [next patch](https://reviewboard.mozilla.org/r/210786/)) that looks in both for files, and prefers c-c paths if the exist. There is an additional wrinkle in how `DIRS` is passed from `moz.build` to the recursive make backend: relative paths in `DIRS` get interpreted relative to the file where it is defined in moz.build, but the objdir of the including moz.build in make (via [here](https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/backend/recursivemake.py#1005-1006), [here](https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/context.py
#674-682) and [here](https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/context.py#713)).

The existing code was all working fine until [this change](https://hg.mozilla.org/mozilla-central/diff/e2b263515feb/dom/canvas/moz.build#l1.12) was introduced, which triggered the heuristic which perfers c-c paths to m-c paths, causing includes like [this](https://hg.mozilla.org/mozilla-central/rev/e2b263515feb#l5.12) to not not work in thunderbird builds. Given that the heuristic is only needed in c-c's `app.mozbuild` files, and everywhere there we know whether to expect the path in c-c or m-c, I though the best solution was simply to get rid of the heuristic and make all the places that want to recurse in to c-c explicit.

Note: all this is only for builds where c-c is located outside the m-c tree, using `--with-external-source-dir` to point to the c-c tree. I hope to be at a point where all this can be torn out sometime shortly after 60 branches.
Comment on attachment 8940579 [details]
Bug 1428608: Allow explicitly specifying dirs from EXTERNAL_SOURCE_DIR to recurse into;

https://reviewboard.mozilla.org/r/210784/#review216834

> This comment feels culted.  Is it correct?

It is copied from the same comment in `SourcePath`. But I beleive it is still accurate. This function exists AFAICT to deal with path oddities around `--with-external-source-dir`. This changes it from a heuristic, to explictly specifying cases.
Comment on attachment 8940578 [details]
Bug 1428608: Avoid capturing mutable `DIRS` list from moz.build context;

https://reviewboard.mozilla.org/r/210782/#review216970

::: python/mozbuild/mozbuild/frontend/emitter.py:1598
(Diff revision 1)
>                      'is not referenced in the moz.build file. '
>                      'Please define JAR_MANIFESTS.', context)
>  
>      def _emit_directory_traversal_from_context(self, context):
>          o = DirectoryTraversal(context)
> -        o.dirs = context.get('DIRS', [])
> +        o.dirs += context.get('DIRS', [])

This should be `o.dirs = list(context.get('DIRS', [])` because it doesn't actually do an append. The operation does a replace, so the assignment operator should be used.
Comment on attachment 8940579 [details]
Bug 1428608: Allow explicitly specifying dirs from EXTERNAL_SOURCE_DIR to recurse into;

https://reviewboard.mozilla.org/r/210784/#review216968

I'm not super thrilled about this. I'll hold off on final review until I hear what glandium has to say, as he was a bit opinionated about path resolution the last time this came up.

::: python/mozbuild/mozbuild/frontend/context.py:1427
(Diff revision 1)
>          above or below. Use ``..`` for parent directories and ``/`` for path
>          delimiters.
>          """),
>  
> +    'EXTERNAL_SOURCE_DIRS': (ContextDerivedTypedList(ExternalSourcePath), list,
> +        """Directories relative to EXTERNAL_SOURCE_DIR.

This should clarify that `EXTERNAL_SOURCE_DIR` refers to the build config variable and not a moz.build variable.

It should also say what is being done with the directories. e.g. does it behave like `DIRS`?

::: python/mozbuild/mozbuild/frontend/emitter.py:1599
(Diff revision 1)
>                      'Please define JAR_MANIFESTS.', context)
>  
>      def _emit_directory_traversal_from_context(self, context):
>          o = DirectoryTraversal(context)
>          o.dirs += context.get('DIRS', [])
> +        o.dirs += context.get('EXTERNAL_SOURCE_DIRS', [])

Someone (I think glandium) did a bunch of work several months back to consolidate all our `*_DIRS` variables and just use `DIRS`. I would prefer to not introduce another `*_DIRS` variant and recycle `DIRS` if possible.
Attachment #8940579 - Flags: review?(gps)
Flags: needinfo?(gps)
Comment on attachment 8940579 [details]
Bug 1428608: Allow explicitly specifying dirs from EXTERNAL_SOURCE_DIR to recurse into;

https://reviewboard.mozilla.org/r/210784/#review219482

cf. the discussion we had on irc, we should remove that LOCAL_INCLUDES of / instead.
Attachment #8940579 - Flags: review?(mh+mozilla) → review-
Attachment #8943412 - Flags: review?(nfroyd)
Please note that the problem I reported got fixed in bug 1428678: https://hg.mozilla.org/mozilla-central/rev/6f5fac320fcb
So I'm not sure whether the export is required.
Severity: blocker → normal
even better!
Comment on attachment 8943412 [details]
Bug 1428608 - Forbid / or !/ in LOCAL_INCLUDES.

https://reviewboard.mozilla.org/r/213736/#review219544

\o/  A test for this would be fantastic, so it's a little harder to sneak this change out, but meh.
Attachment #8943412 - Flags: review?(nfroyd) → review+
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/ca36a6c4f8a4
Forbid / or !/ in LOCAL_INCLUDES. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/ca36a6c4f8a4
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 59.0
Attachment #8940578 - Attachment is obsolete: true
Attachment #8940579 - Attachment is obsolete: true
Attachment #8940580 - Attachment is obsolete: true
Attachment #8940581 - Attachment is obsolete: true
Attachment #8940581 - Flags: review?(core-build-config-reviews)
I can't recall why I needinfo'd myself.
Flags: needinfo?(gps)
Assignee: nobody → mh+mozilla
You need to log in before you can comment on or make changes to this bug.