Closed Bug 1035599 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla34

People

(Reporter: glandium, Assigned: jcranmer)

References

Details

Attachments

(3 files, 3 obsolete files)

Attached 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
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.
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.
Attached 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)
Attached 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+
A little bit more m-c changes to fix the inclusion of the branding configure.sh.
Attachment #8467390 - Flags: review?(mh+mozilla)
Attachment #8466875 - Flags: review?(mh+mozilla) → review?(gps)
Attachment #8467390 - Flags: review?(mh+mozilla) → review+
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?
> 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+
(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.
Attachment #8466875 - Attachment is obsolete: true
Attachment #8468157 - Flags: review?(gps)
(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+
Attachment #8467390 - Flags: checkin+
Attachment #8468157 - Flags: checkin+
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+
(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
Blocks: 1052985
QA Whiteboard: [qa-]
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: