1035599 - Pseudo-merge m-c and c-c's objdir

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

Attachment: Patch

Essentially a dump if anyone wants to take it further, because I won't. The attached patch makes the build work for me on linux by doing the following:

- Create objdir
- Run m-c's configure with --enable-application=../mail --with-external-source-dir=..

There is no actual integration with the c-c build system, and there are very likely issues with mozconfigs, as this makes the topsrcdir of the build is m-c.

Note that on c-c's end, this requires this patch to build: https://bugzilla.mozilla.org/attachment.cgi?id=8376652

Comment 1

[Mike Hommey [:glandium]]

Note it should be possible to go without the recursivemake.py changes with more changes on c-c's end, and that would make the c-c tree use the m-c config/*.mk.

Comment 2

[Joshua Cranmer [:jcranmer]]

Okay, since I happen to unexpectedly have a bit of time on my hands at the moment, I'm going to try pushing forward on this.

Originally, I wanted to tweak these patches so that $(topsrcdir) pointed to mozilla-central instead of comm-central. That turned out to be a bit harder than I expected, so I'll separate that step into another patch.

Cc'ing a few people who may want to be aware that this will likely require investigation on their part to make sure that builders keep working.

Comment 3

[Joshua Cranmer [:jcranmer]]

Attachment: Mozilla-central changes

This also ought to fix the issue with Windows and OS X builds with the /Makefile.in change.

Comment 4

[Joshua Cranmer [:jcranmer]]

Attachment: Comm-central changes

Parts of this patch are leveraged from the cc-rework bug, particularly the changes for packaging tests and some of the global path renamings.

I have not tested that SeaMonkey and Instantbird work with this patch. I also haven't test l10n repacks because I have no clue how to run those.

On the other hand, this appears to solve all outstanding build issues on comm-central.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8466878 [details] [diff] [review]
Comm-central changes

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

Ok, I can't get this to build locally, mainly due to bug 1040009. However the changes & the fact there's a try build seem to indicate its certainly a lot better than what we have. We can monitor l10n builds as we go.

Comment 6

[Joshua Cranmer [:jcranmer]]

Attachment: Fix MOZ_APP_DISPLAYNAME

A little bit more m-c changes to fix the inclusion of the branding configure.sh.

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8466875 [details] [diff] [review]
Mozilla-central changes

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

::: Makefile.in
@@ +327,5 @@
>  ifeq ($(MOZ_WIDGET_TOOLKIT),gtk3)
>  toolkit/library/target: widget/gtk/mozgtk/gtk3/target
>  endif
>  ifdef MOZ_LDAP_XPCOM
> +ldap/target: security/build/target mozglue/build/target

isn't this the fix for a separately filed bug?

Comment 8

[Mike Hommey [:glandium]]

> Flags: review?([e-mail]) → review?([e-mail])

In case it isn't clear why, this was discussed on irc, and the reason is that the patch is largely mine (attachment 8452099 [details] [diff] [review]) rebased, with a Makefile.in change.

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8466875 [details] [diff] [review]
Mozilla-central changes

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

This is on the right track, but not quite ready for r+.

::: configure.in
@@ +2523,5 @@
>      ;;
>    *)
>      case $GCC_VERSION in
>      4.4*|4.6*)
> +      VISIBILITY_FLAGS='-I$(DIST)/system_wrappers -include $(MOZILLA_DIR)/config/gcc_hidden_dso_handle.h'

Does MOZILLA_DIR exist in m-c?

::: python/mozbuild/mozbuild/frontend/reader.py
@@ +139,5 @@
>          topobjdir = mozpath.abspath(config.topobjdir)
>          topsrcdir = config.topsrcdir
>          norm_topsrcdir = mozpath.normpath(topsrcdir)
>  
> +        self.externals = []

Could we make this self.external_source_dirs? "externals" feels a bit ambiguous to me.

@@ +205,4 @@
>          """
>          if os.path.isabs(path):
> +            if filesystem_absolute:
> +                return path

Do we need to normalize the returned path?

@@ +206,5 @@
>          if os.path.isabs(path):
> +            if filesystem_absolute:
> +                return path
> +            for root in [self.topsrcdir] + self.externals:
> +                p = mozpath.normpath(mozpath.join(root, path[1:]))

Please add a comment that the [1:] is to work around .join()'s behavior of ignoring previous components if an absolute path is encountered.

Also, what happens if path is a Windows style path? Perhaps we should mozpath.normpath(path) or at least assert path[0] == '/'

@@ +209,5 @@
> +            for root in [self.topsrcdir] + self.externals:
> +                p = mozpath.normpath(mozpath.join(root, path[1:]))
> +                if os.path.exists(p):
> +                    return p
> +            return mozpath.normpath(mozpath.join(self.topsrcdir, path[1:]))

Ditto.

@@ +228,4 @@
>  
>          # realpath() is needed for true security. But, this isn't for security
>          # protection, so it is omitted.
> +        normalized_path = self.normalize_path(path, filesystem_absolute)

Nit: filesystem_absolute should be passed as a named argument, not as a positional argument.

@@ +883,5 @@
>                              'Tier directory (%s) registered multiple '
>                              'times in %s' % (d, tier), sandbox)
>                      recurse_info[d] = {'tier': tier,
>                                         'parent': sandbox['RELATIVEDIR'],
> +                                       'var': 'TIER_DIRS'}

TIER_DIRS doesn't exist.

Comment 10

[Joshua Cranmer [:jcranmer]]

(In reply to Gregory Szorc [:gps] from comment #9)
> Does MOZILLA_DIR exist in m-c?

Yes.
> @@ +205,4 @@
> >          """
> >          if os.path.isabs(path):
> > +            if filesystem_absolute:
> > +                return path
> 
> Do we need to normalize the returned path?

According to glandium, no. The function will only be called with purprortedly topsrcdir-relative-with-/ paths.

Comment 11

[Joshua Cranmer [:jcranmer]]

Attachment: Mozilla-central changes

Comment 12

[Mike Hommey [:glandium]]

(In reply to Joshua Cranmer [:jcranmer] (high latency until August 11) from comment #10)
> (In reply to Gregory Szorc [:gps] from comment #9)
> > Does MOZILLA_DIR exist in m-c?
> 
> Yes.
> > @@ +205,4 @@
> > >          """
> > >          if os.path.isabs(path):
> > > +            if filesystem_absolute:
> > > +                return path
> > 
> > Do we need to normalize the returned path?
> 
> According to glandium, no. The function will only be called with
> purprortedly topsrcdir-relative-with-/ paths.

Which, after reading further, I figured is not true, but it doesn't matter anyways. The function is either called with a filesystem absolute path in which case the path we do want is the path that was given in, or a topsrcdir-absolute-or-relative path with forward slashes, in which case, it's mozpath.joined top the relevant directory (and in which case path[1:] works because it's a topsrcdir-absolute path, never a windows path.
The result of that function is expected to be a system path, not a moz.build path, so it needs no normalization, and the required normalization is done many times by whatever uses the result of the function.

Comment 13

[Joshua Cranmer [:jcranmer]]

M-i checkins:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e234b61f711e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/698584a65cdb

Comment 14

[Joshua Cranmer [:jcranmer]]

Attachment: Comm-central changes

This is the patch I intend to check in when m-i merges to m-c. It's fixed for the bitrot that glandium added, and has a few other changes to make Instantbird and Seamonkey build, approved by Florian and IanN.

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e234b61f711e
https://hg.mozilla.org/mozilla-central/rev/698584a65cdb
https://hg.mozilla.org/mozilla-central/rev/7dc089c4bd76

Comment 16

[Joshua Cranmer [:jcranmer]]

And here's the c-c changeset:
https://hg.mozilla.org/comm-central/rev/949e0465b1c3

Comment 17

[Florian Quèze [:florian]]

(In reply to Joshua Cranmer [:jcranmer] from comment #16)
> And here's the c-c changeset:
> https://hg.mozilla.org/comm-central/rev/949e0465b1c3

Fix for Mac universal builds: https://hg.mozilla.org/comm-central/rev/cb779d2d4181

Comment 18

[Florian Quèze [:florian]]

Fixed a few issues in im/build.mk: https://hg.mozilla.org/comm-central/rev/3c377dcfb242