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

RESOLVED FIXED in mozilla34

Status

defect
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: glandium, Assigned: jcranmer)

Tracking

unspecified
mozilla34
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Posted patch Patch (obsolete) — Splinter Review
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
(Reporter)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
Posted patch Mozilla-central changes (obsolete) — Splinter Review
This also ought to fix the issue with Windows and OS X builds with the /Makefile.in change.
Assignee: nobody → Pidgeot18
Attachment #8452099 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8466875 - Flags: review?(mh+mozilla)
(Assignee)

Comment 4

5 years ago
Posted patch Comm-central changes (obsolete) — Splinter Review
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.
Attachment #8466878 - Flags: review?(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.
Attachment #8466878 - Flags: review?(standard8) → review+
(Assignee)

Comment 6

5 years ago
A little bit more m-c changes to fix the inclusion of the branding configure.sh.
Attachment #8467390 - Flags: review?(mh+mozilla)
(Assignee)

Updated

5 years ago
Attachment #8466875 - Flags: review?(mh+mozilla) → review?(gps)
(Reporter)

Updated

5 years ago
Attachment #8467390 - Flags: review?(mh+mozilla) → review+
(Reporter)

Comment 7

5 years ago
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?
(Reporter)

Comment 8

5 years ago
> Flags: review?(mh+mozilla@glandium.org) → review?(gps@mozilla.com)

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 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.
Attachment #8466875 - Flags: review?(gps) → feedback+
(Assignee)

Comment 10

5 years ago
(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.
(Assignee)

Comment 11

5 years ago
Attachment #8466875 - Attachment is obsolete: true
Attachment #8468157 - Flags: review?(gps)
(Reporter)

Comment 12

5 years ago
(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.
Attachment #8468157 - Flags: review?(gps) → review+
(Assignee)

Updated

5 years ago
Attachment #8467390 - Flags: checkin+
(Assignee)

Updated

5 years ago
Attachment #8468157 - Flags: checkin+
(Assignee)

Comment 14

5 years ago
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.
Attachment #8466878 - Attachment is obsolete: true
Attachment #8469729 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e234b61f711e
https://hg.mozilla.org/mozilla-central/rev/698584a65cdb
https://hg.mozilla.org/mozilla-central/rev/7dc089c4bd76
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
(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

Updated

5 years ago
Blocks: 1052985
QA Whiteboard: [qa-]

Updated

a year ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.