1122812 - Change the visual studio project generator to add the Unified_foo_bar_baz0.cpp files to the project, and not the sources specified in UNIFIED_SOURCES

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(no longer active)]

This should fix the issues with removing non-unified builds that people are reporting.

Comment 1

[Nathan Froyd [:froydnj]]

I will do this, I already have some patches written in my head.

Comment 2

[(no longer active)]

Thank you!

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

How will we handle changes that modify the number of generated cpp files?

Comment 4

[(no longer active)]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #3)
> How will we handle changes that modify the number of generated cpp files?

It won't.  The project needs to be reconstructed in that case anyway, right?

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: part 1 - move unified file grouping to UnifiedSources's constructor

UnifiedSources, not the recursivemake backend, should be responsible for
figuring out what unified files to generate, and what those unified
files should contain.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: part 2 - factor out the grabbing of an object's backend.mk

UnifiedSources will be processed outside of consume_object, so we need
some way of accessing the backend file for an object outside of
consume_object as well.

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: part 3 - move unified files writing for UnifiedSources to CommonBackend

CommonBackend is where the writing of the unified files belong, since
that's an operation common to all backends.  A special hook into
subclasses is used to enable subclass-specific processing of
UnifiedSources.

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: part 4 - update the Visual Studio backend for UnifiedSources changes

With the previous changes, we can now tell Visual Studio about the
actual unified files, rather than the files that are #include'd in them.
I believe this is much closer to what Visual Studio wants to see, and
enables things like Intellisense to work properly.

Comment 9

[Nathan Froyd [:froydnj]]

This series follows bug 1129635 comment 0's suggestion; I don't know if there's a better way to do it, but I couldn't figure out a pleasant way to do it within the constraints assumed by consume_object and the .ack() flag on a moz.build object.

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: part 4 - update the Visual Studio backend for UnifiedSources changes

With the previous changes, we can now tell Visual Studio about the
actual unified files, rather than the files that are #include'd in them.
I believe this is much closer to what Visual Studio wants to see, and
enables things like Intellisense to work properly.

This version of the patch actually ensures that Unified*.cpp files are written
into the vcxproj files.  The visualstudio backend doesn't follow the
consume_object protocol (insofar as we have one...) and processes only the few
frontend object types it's interested in, ack()'ing all the rest.  This means
that CommonBackend.consume_object never gets called, so the
_process_unified_sources hook never gets called.

What this also means is that building the Visual Studio backend on its own
won't work without building the recursive make backend first.  I don't think
this is a problem currently, as many other things won't work with the Visual
Studio backend (e.g. WebIDL/IPDL/XPIDL), but it's good to note it here for
completeness.

Comment 11

[Michael Shal [:mshal]]

Comment on attachment 8559991 [details] [diff] [review]
part 3 - move unified files writing for UnifiedSources to CommonBackend

>diff --git a/python/mozbuild/mozbuild/backend/common.py b/python/mozbuild/mozbuild/backend/common.py
>index 90e7189..992fd40 100644
>--- a/python/mozbuild/mozbuild/backend/common.py
>+++ b/python/mozbuild/mozbuild/backend/common.py
>+        elif isinstance(obj, UnifiedSources):
>+            if obj.have_unified_mapping:
>+                self._write_unified_files(obj.unified_source_mapping, obj.objdir)

>diff --git a/python/mozbuild/mozbuild/backend/recursivemake.py b/python/mozbuild/mozbuild/backend/recursivemake.py
>index ebb7038..e62775d 100644
>--- a/python/mozbuild/mozbuild/backend/recursivemake.py
>+++ b/python/mozbuild/mozbuild/backend/recursivemake.py
>+        if obj.have_unified_mapping:
>+            self._write_unified_files(obj.unified_source_mapping, backend_file.objdir)

Why are both recursivemake.py and common.py writing out the unified files? It looks like they both are trying to write out the same files when generating the make backend -- in an strace, I can see Unified_XYZ.cpp is opened twice - once to write the file and once to read it & compare, or opened for reading twice if it already exists. I think we would just want to do that in common.py, right?

Comment 12

[Michael Shal [:mshal]]

Comment on attachment 8561613 [details] [diff] [review]
part 4 - update the Visual Studio backend for UnifiedSources changes

>diff --git a/python/mozbuild/mozbuild/backend/visualstudio.py b/python/mozbuild/mozbuild/backend/visualstudio.py
>index 7f0cfac..69e3ffa 100644
>--- a/python/mozbuild/mozbuild/backend/visualstudio.py
>+++ b/python/mozbuild/mozbuild/backend/visualstudio.py
>@@ -105,17 +105,19 @@ class VisualStudioBackend(CommonBackend):
>         elif isinstance(obj, UnifiedSources):
>-            self._add_sources(reldir, obj)
>+            # XXX we should be letting CommonBackend.consume_object call this
>+            # for us instead.
>+            self._process_unified_sources(obj);

So why doesn't CommonBackend.consume_object work here?

Comment 13

[Nathan Froyd [:froydnj]]

Attachment: part 3 - move unified files writing for UnifiedSources to CommonBackend

CommonBackend is where the writing of the unified files belong, since
that's an operation common to all backends.  A special hook into
subclasses is used to enable subclass-specific processing of
UnifiedSources.

Now with the _write_unified_files call removed from recursivemake.py.

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to Michael Shal [:mshal] from comment #11)
> Why are both recursivemake.py and common.py writing out the unified files?
> It looks like they both are trying to write out the same files when
> generating the make backend -- in an strace, I can see Unified_XYZ.cpp is
> opened twice - once to write the file and once to read it & compare, or
> opened for reading twice if it already exists. I think we would just want to
> do that in common.py, right?

Yeah, I just fail at moving the code out of recursivemake.py.  New patch up.

(In reply to Michael Shal [:mshal] from comment #12)
> Comment on attachment 8561613 [details] [diff] [review]
> part 4 - update the Visual Studio backend for UnifiedSources changes
> 
> >diff --git a/python/mozbuild/mozbuild/backend/visualstudio.py b/python/mozbuild/mozbuild/backend/visualstudio.py
> >index 7f0cfac..69e3ffa 100644
> >--- a/python/mozbuild/mozbuild/backend/visualstudio.py
> >+++ b/python/mozbuild/mozbuild/backend/visualstudio.py
> >@@ -105,17 +105,19 @@ class VisualStudioBackend(CommonBackend):
> >         elif isinstance(obj, UnifiedSources):
> >-            self._add_sources(reldir, obj)
> >+            # XXX we should be letting CommonBackend.consume_object call this
> >+            # for us instead.
> >+            self._process_unified_sources(obj);
> 
> So why doesn't CommonBackend.consume_object work here?

Because visualstudio.py never calls CommonBackend.consume_object.  I think this should be fixed, but it may also be part of a larger fix deciding how we want the common backend and sub-backends to work together.

Comment 15

[Michael Shal [:mshal]]

Comment on attachment 8562816 [details] [diff] [review]
part 3 - move unified files writing for UnifiedSources to CommonBackend

Looks good to me!

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/308e00e6f6e8
https://hg.mozilla.org/mozilla-central/rev/65b27bd85568
https://hg.mozilla.org/mozilla-central/rev/732b88e667d0
https://hg.mozilla.org/mozilla-central/rev/b1da75458e00

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

This hurts my productivity greatly as it breaks intellisense.

Can we back this out? At least until we can provide a command line option so that we can generate a project in which intellisense and all the other awesome code navigation things works?

Comment 18

[Nathan Froyd [:froydnj]]

(In reply to Chris Pearce (:cpearce) from comment #17)
> This hurts my productivity greatly as it breaks intellisense.
> 
> Can we back this out? At least until we can provide a command line option so
> that we can generate a project in which intellisense and all the other
> awesome code navigation things works?

Sorry, I meant to address this in bug 1138250 yesterday.  I'll do that today.