Closed Bug 648979 (ccrework) Opened 13 years ago Closed 6 years ago

Rework comm-central to build underneath mozilla-central

Categories

(MailNews Core :: Build Config, defect)

defect
Not set
blocker

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1366607

People

(Reporter: standard8, Assigned: jcranmer)

References

(Depends on 1 open bug, Blocks 1 open bug, )

Details

Attachments

(4 files, 4 obsolete files)

Since it was set up, the comm-central build system has served us well.

However the primary issue we have with it is that it requires us to constantly port across build system changes from mozilla-central. 

If we can make direct use of the mozilla-central build system, then we don't need to copy it for the comm-central apps all the time.

Therefore the proposal is that we move comm-central to be underneath mozilla-central (this does not mean that we will be inside mozilla-central). So the directory structure we have now is:

/comm-central/mailnews
/comm-central/mail
/comm-central/mozilla/accessible
/comm-central/mozilla/toolkit
etc.

where /mozilla/ is the directory where the hg clone of mozilla-central is held.

Under the new system we'd have:

/mozilla-central/accessible
/mozilla-central/toolkit
/mozilla-central/comm/mailnews
/mozilla-central/comm/mail
etc.

We'd need mozilla-central's configure and a few other items to have some additional hooks so that we could still specify options for comm-central (e.g. building with ldap), however ted and khuey agreed that it'd be a reasonable addition to do.
Depends on: 648980
I have a first draft for this. I'm currently looking at getting the patches or repo published somewhere.
I can now post the said patches. Some notes before I do that:

- I currently assume that comm-central is in a sub-directory of mozilla called 'comm'.

- I've written a script which does the main bulk of replacements that are necessary. It isn't optimised and the regexps could probably be better, but it works.

- I've got an additional patch which starts getting things working - with both the script and the patch I can get Thunderbird building and running on my mac (in 64 bit mode only), and it also passes at least some of the xpcshell-tests.

- There's much more work to do, including getting SeaMonkey and Lightning building & running.

- We'll also need the hooks for configure.in options. So far the things I know that we'll need some kind of hooks for are:
** Running LDAP sub-configures and getting variables passed in.
** Getting MOZ_STATIC_MAIL_BUILD and MOZ_COMPOSER defined in the build system.
** Properly getting things like MOZ_MAIL_NEWS defined in the build system.
(oh and the builds are currently without LDAP due to the sub-configures issues).
Attached file Replacement script (obsolete) —
This is the script to do all the replacements. cd into the comm-central directory and sh ./translatescript.sh
Attachment #526800 - Attachment is patch: false
This is the patch that is generated as a result of running the replacement script.
Attached patch Manual Changes WIP v1 (obsolete) — Splinter Review
This is the first round of manual changes.

Note: to get everything to compile, I also add the following to the mozilla-central autoconf.mk.in:

MOZ_STATIC_MAIL_BUILD = 1
MOZ_COMPOSER = 1
I wonder if it would make sense to have a clone of comm-central in your user space where you add those patches, and merge stuff in from c-c every now and then, and we can use it as a "project branch" and try to run builds of different configs and systems against to test stuff.

(In reply to comment #6)
> Note: to get everything to compile, I also add the following to the
> mozilla-central autoconf.mk.in:
> 
> MOZ_STATIC_MAIL_BUILD = 1
> MOZ_COMPOSER = 1

That's something we really need to find a way for avoiding, we should not (need to) pollute mozilla trees with comm-specific code/vars.
(In reply to comment #7)
> I wonder if it would make sense to have a clone of comm-central in your user
> space where you add those patches, and merge stuff in from c-c every now and
> then, and we can use it as a "project branch" and try to run builds of
> different configs and systems against to test stuff.

Yeah, I've kinda been debating that. I'm not sure if to have a patch queue stored or a branch. I guess a branch would work for review before merge as the changes shouldn't be that big (generally I think project branches work better if the changes are reviewed as they go in - do we want to do that here?).

> (In reply to comment #6)
> > Note: to get everything to compile, I also add the following to the
> > mozilla-central autoconf.mk.in:
> > 
> > MOZ_STATIC_MAIL_BUILD = 1
> > MOZ_COMPOSER = 1
> 
> That's something we really need to find a way for avoiding, we should not (need
> to) pollute mozilla trees with comm-specific code/vars.

That's covered in the last point of comment 8.
(In reply to comment #8)
> Yeah, I've kinda been debating that. I'm not sure if to have a patch queue
> stored or a branch. I guess a branch would work for review before merge as the
> changes shouldn't be that big (generally I think project branches work better
> if the changes are reviewed as they go in - do we want to do that here?).

Having review of the principles should be good, though I'm not sure if review on every single line is needed, and to some degree, even review-after-the-fact works nice if a project branch is so confined to a specific purpose and the actual changes should be easy to sort out.

> That's covered in the last point of comment 8.

This is comment 8, I guess you mean comment 2, actually ;-)
Blocks: 599615
Comment on attachment 526800 [details]
Replacement script

># Replace '(/mail' with '(/comm/mail' in jar.mn files
>find . -name 'jar.mn' -exec sed -i backup -E 's/(.*)\(\/mail(.*)/\1\(\/comm\/mail\2/' {} \;

Merely looking at this script, shouldn't we do this step for suite, and similar steps for calendar?
Probably, I just wasn't concentrating on getting them working at the time.

Still hoping to set up a test repo for this, just working on fixing another bug first.
Hm, sorry, I am new to this bug, and this discussion is interesting, but just a hair's width above my head.

Does all this mean that if and when this bug gets FIXED, the mozilla-central client.py will (at least when the mozconfig says we're building the suite) pull all six of the mozilla-central, comm-central, chatzilla, venkman, LDAP SDK and DOMi repositories (like the comm-central client.py does now)? Or will there still be a comm-central client.py, which will (among other things) pull mozilla-central a directory level or so above itself?
Undetermined.

So far, it seems like we'll make m-c client.py do most of what c-c client.py does. Ted was -- in theory -- ok with that plan. But first we want to get this setup working before we delve to the client.py side.
I've now created a repository where we can work on this and try and get things running correctly.

As I think it would be useful to have some easy co-ordination on this, I've created an etherpad here:

http://etherpad.mozilla.com:9000/ccrework

with notes on how to build, what work needs doing etc. Please keep up to date.

I'm proposing that build config peers can land changes without review at this time. If non-peers wish to land, then please get a general once-over from a peer so that we can check things are heading in the right direction.
Flags: in-testsuite-
Version: unspecified → Trunk
No longer blocks: 599615
Blocks: 599615
Depends on: 781446
Depends on: 846540
I've successfully built a version of Thunderbird that uses the m-c build system by doing the following things:
1. Introduce lots of symlinks everywhere, to avoid sed path changes. Basically:
c-c/mozilla/comm -> c-c
c-c/mozilla/mozilla -> c-c/mozilla
c-c/mozilla/{mail,mailnews,chat} - c-c/*
c-c/mozilla/editor/ui -> c-c/editor/ui
2. Fix paths in bridge/bridge.mozbuild, mail/app.mozbuild, and mail/configure.in
3. Flatten everything in mail/app.mozbuild
4. Disable mozbuild-migration checks in the m-c build system, as well as the xpccheck stuff
5. s/$(DIR_INSTALL)/$(INSTALL)/g in comm-central.
6. Added MOZDEPTH/MOZILLA_SRCDIR to mozilla-central/config/baseconfig.mk

From the results of this, after bug 846540 lands, the following things would need to be done:
1. Change DIR_INSTALL to INSTALL in the c-c build system (this can be done before)
2. Find some way to make xpcshell and mozmill tests work again. For xpcshell, we may have to push on making a non-broken global manifest system (bug 844655 disavows fixing that as a goal right now). For mozmill, we at least need to integrate it into top-level make/mach, and make package-tests as well.
3. Fix mail/app.mozbuild and suite/app.mozbuild, as well as the respective configure.ins, to get paths right and eliminate the COMM_BUILD duality. We can probably eliminate bridge/bridge.mozbuild at that time too, but it's not a big deal.
4. Complete all pending mozbuild migration work.
5. Break out a sed script and rewrite all critical pseudo-absolute paths.
6. Change all the builders and release engineering scripts.

I've done no testing other than make, make package-tests, and running thunderbird to see if it keels over dead so far, so it's quite possible we will have issues with things like l10n-repacks or packaging scripts. I'll let someone who knows that stuff better than I worry about those changes.
Depends on: 864191
Depends on: 869449
Depends on: 869635
Depends on: 891240
Blocks: 891240
No longer depends on: 891240
Depends on: 896209
Depends on: 901572
Status update for people following along at home:
My current patch-queue is capable of making TB (without Lightning) build under mozilla-central. The main patches that can be done before the move have been uploaded and awaiting review (basically, more moz.build migration and some makefile cleanup).

The stuff that can't be done before the move are the massive path-rewriting, which is this:
find -name Makefile.in | grep -v mozilla | grep -v ldap/sdks | xargs sed -e '/include/!s+$(topsrcdir)+$(topsrcdir)/comm+g' -i
find -name Makefile.in | grep -v mozilla | grep -v ldap/sdks | xargs sed -e 's+$(topsrcdir)/comm/mozilla+$(topsrcdir)+g' -i
find -name Makefile.in | grep -v mozilla | grep -v ldap/sdks | xargs sed -e '/config\/autoconf.mk/!s+$(DEPTH)/\([^$]\)+$(DEPTH)/comm/\1+g' -i
find -name Makefile.in | grep -v mozilla | grep -v ldap/sdks | xargs sed -e 's+$(MOZDEPTH)+$(DEPTH)+g' -i
find -name Makefile.in | grep -v mozilla | grep -v ldap/sdks | xargs sed -e 's+$(MOZILLA_SRCDIR)+$(topsrcdir)+g' -i
find -name jar.mn | grep -v mozilla | grep -v ldap/sdks | xargs sed -e 's+(/+(/comm/+' -i
find -name jar.mn | grep -v mozilla | grep -v ldap/sdks | xargs sed -e 's+(/comm/mozilla+(/mozilla+' -i
find -name moz.build | grep -v mozilla | xargs sed -e 's+$(topsrcdir)+$(topsrcdir)/comm+g' -i

Then there are several manual path changes to account for things not easily findable (like configure.in and bridge.mozbuild changes).

The final step is to kill COMM_BUILD.

As of right now, Thunderbird builds, but package-tests and l10n-check break, as do all of the comm-central specific test files.
Depends on: 944952
Depends on: 957220
Any relative obj dirs, e.g. MOZ_OBJDIR=@TOPSRCDIR@/../opt/ or just /opt/, are broken for me. Folks on #maildev told me that this bug fixes it.
Workaround: absolute paths work, but they are cumbersome when you have many trees.
Severity: normal → blocker
Attached file merge-script.sh
This is the final script I'll be using (minus applying any patches named temp-*.diff). Several of the steps are autogenerated by running commands instead of manually writing patches that will tend to get obsolete. I've separated out each step into different commits so that it's clear what all the changes have in common, but this does break pretty severely the rule that individual patches should work on their own.
Assignee: nobody → Pidgeot18
Status: NEW → ASSIGNED
Attachment #8376275 - Flags: review?(mbanner)
This is the file named rewrite-paths.diff mentioned in the merge script. It contains the manual changes not covered by the automatic rewrite, as well as moving the bridge.mozbuild file to mailnews.mozbuild to eliminate a useless and potentially confusing top-level directory.

Something else that could be done is to move the mork logic under mailnews/db, but since m-c already has a top-level db/ directory, I don't think it's quite so necessary for the merge work.
Attachment #526800 - Attachment is obsolete: true
Attachment #526803 - Attachment is obsolete: true
Attachment #526805 - Attachment is obsolete: true
Attachment #8376652 - Flags: review?(mbanner)
This is the package-tests.diff file, which mostly contains manual fixes to get packaging to work properly, most of which cannot be applied prior to this bug because it breaks under the current build system.
Attachment #8376653 - Flags: review?(mbanner)
Attached patch m-c-post.diff (obsolete) — Splinter Review
This is m-c-post.diff, the changes we need to mozilla-central to make this work. I'm not asking for review yet because I'm not sure whom to ask, and I also expect this code to be more likely to bitrot than the other files. Effectively, we need a dependency to make the pseudo-derecurse work--this is the default in m-c and will I believe eventually be the only supported mode; it was never ported to c-c because both glandium and I gave up trying to figure out why it wasn't working.

The OS X universal build changes are ugly, but after discussing with Ted in #build, we came to the conclusion that adding yet-another-directory to the list was the way to go.
In terms of testing:
The Thunderbird code is tested by periodic pushes to Alder acting as a try server for this bug. This also confirms the buildability of Lightning for TB, although we don't run any Lightning tests on buildbot.

I've manually built SeaMonkey on one of the attempted heads (after dropping inspector, venkman, and chatzilla in their appropriate places), and that worked, and I've built it on the latest change without the three extensions, and that worked as well. Instantbird doesn't build for me on Linux with comm-central, but it builds just as well on the merged repository, so I'll take that as a sign that it should work.

The manual builds are done as regular ./mach builds only on Linux. My experience with build system work is that mailnews, calendar, and mail are all far nastier in stressing the build system than chat, im, and suite, so I feel safe in assuming that works-on-Linux for SM and IB means works-on-all-platforms. I'm not so confident that packaging works properly for SM and IB, but I assume that packaging issues are not quite so high a priority with both IB and SM's buildbots being down and they don't preclude people working on code locally. I'll be happy to work with people to fix build errors introduced by this change.

I haven't tried Firefox on it yet, but I will be sure to do a real try run before getting review on m-c-post.diff and certainly before pushing anything to m-c. :-)
Comment on attachment 8376654 [details] [diff] [review]
m-c-post.diff

I think you should ask gps for review on the m-c bit as he is the build system owner.
Comment on attachment 8376654 [details] [diff] [review]
m-c-post.diff

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

::: Makefile.in
@@ +294,5 @@
>  ifdef ENABLE_CLANG_PLUGIN
>  js/src/export config/export: build/clang-plugin/export
>  endif
> +ifdef MOZ_LDAP_XPCOM
> +ldap/export: config/export

This shouldn't be needed. The backend makes */export depend on config/export already.
Comment on attachment 8376275 [details]
merge-script.sh

I've not tested it, but it looks fine.
Attachment #8376275 - Flags: review?(standard8) → review+
Attachment #8376652 - Flags: review?(standard8) → review+
Attachment #8376653 - Flags: review?(standard8) → review+
Patches look good, though obviously we need to move forward with the discussions about when we can land this etc.
Attached patch m-c-post.diffSplinter Review
This is an updated version of the m-c-post.diff patch taking into account glandium's prior comment. I'm also considering this review a chance to have m-c build system people weigh on the patches here.

It passes both alder and try (https://tbpl.mozilla.org/?tree=Try&rev=c5b6d5e4ee1a).
Attachment #8376654 - Attachment is obsolete: true
Attachment #8383181 - Flags: review?(mh+mozilla)
Attachment #8383181 - Flags: feedback?(gps)
Attachment #8383181 - Flags: review?(mh+mozilla) → review+
Attachment #8383181 - Flags: feedback?(gps) → feedback+
See Also: → comm-taskcluster
Depends on: 1384319
Depends on: comm-taskcluster
Depends on: 1421012
Depends on: 1424171
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
So in what way is this fixed?
This was fixed in Bug 1366607.
Resolution: FIXED → DUPLICATE
You need to log in before you can comment on or make changes to this bug.