Closed Bug 775670 Opened 9 years ago Closed 9 years ago

Build Thunderbird using clang

Categories

(Release Engineering :: General, defect, P1)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jhopkins, Assigned: jhopkins)

References

Details

Attachments

(5 files, 2 obsolete files)

comm-central is currently closed due to the following error:

make -C config export
nsinstall.c
/builds/slave/tb-c-cen-osx64/build/mozilla/clang/bin/clang -o host_nsinstall.o -c  -Wall -Wpointer-arith -Wdeclaration-after-statement -Werror=return-type -Wempty-body -Wno-unused -Wno-overlength-strings -Wcast-align -isysroot /Developer/SDKs/MacOSX10.6.sdk -fno-strict-aliasing -ffunction-sections -fdata-sections -pthread -DNO_X11 -pipe  -DNDEBUG -DTRIMMED -g -O3 -fomit-frame-pointer -DXP_UNIX -DXP_MACOSX -DNO_X11 -O3  -DUNICODE -D_UNICODE  -I/builds/slave/tb-c-cen-osx64/build/mozilla/config -I. -I../dist/include  -I/builds/slave/tb-c-cen-osx64/build/objdir-tb/i386/mozilla/dist/include/nspr -I/builds/slave/tb-c-cen-osx64/build/objdir-tb/i386/mozilla/dist/include/nss     -I/builds/slave/tb-c-cen-osx64/build/objdir-tb/i386/mozilla/dist/include/nspr /builds/slave/tb-c-cen-osx64/build/mozilla/config/nsinstall.c
make[7]: /builds/slave/tb-c-cen-osx64/build/mozilla/clang/bin/clang: No such file or directory
pathsub.c
/builds/slave/tb-c-cen-osx64/build/mozilla/clang/bin/clang -o host_pathsub.o -c  -Wall -Wpointer-arith -Wdeclaration-after-statement -Werror=return-type -Wempty-body -Wno-unused -Wno-overlength-strings -Wcast-align -isysroot /Developer/SDKs/MacOSX10.6.sdk -fno-strict-aliasing -ffunction-sections -fdata-sections -pthread -DNO_X11 -pipe  -DNDEBUG -DTRIMMED -g -O3 -fomit-frame-pointer -DXP_UNIX -DXP_MACOSX -DNO_X11 -O3  -DUNICODE -D_UNICODE  -I/builds/slave/tb-c-cen-osx64/build/mozilla/config -I. -I../dist/include  -I/builds/slave/tb-c-cen-osx64/build/objdir-tb/i386/mozilla/dist/include/nspr -I/builds/slave/tb-c-cen-osx64/build/objdir-tb/i386/mozilla/dist/include/nss     -I/builds/slave/tb-c-cen-osx64/build/objdir-tb/i386/mozilla/dist/include/nspr /builds/slave/tb-c-cen-osx64/build/mozilla/config/pathsub.c
make[7]: /builds/slave/tb-c-cen-osx64/build/mozilla/clang/bin/clang: No such file or directory
make[7]: *** [host_nsinstall.o] Error 1
make[7]: *** Waiting for unfinished jobs....
make[7]: *** [host_pathsub.o] Error 1
make[6]: *** [export_tier_base] Error 2
make[5]: *** [tier_base] Error 2
make[4]: *** [default] Error 2
make[3]: *** [default] Error 2
make[2]: *** [build] Error 2
make[1]: *** [build] Error 2
make: *** [build] Error 2
program finished with exit code 2

We will need to get Thunderbird using tooltool and clang to resolve this.

See also: bug 672210, bug 774462, bug 774796
One of the problems which prevents us using Firefox tooltool manifests is the fact that we don't have mozilla tree checked out when we run the tooltool step (the tree is checked out by "compile" step).

Additionally tooltool work in the "build" directory by default, so it expands clang to build/clang, while the mozcofigs points us to $topsrcdir/clang which is biuld/mozilla/clang in Thunderbird case. Probably we'll need to tell setup.sh to create a symlink.
Attached patch buildbot-configs patch (obsolete) — Splinter Review
Comment on attachment 644034 [details] [diff] [review]
buildbot-configs patch

You don't need to define tooltool_manifest_src per branch anymore. Define those per platform in PLATFORM_VARS and you are done. http://hg.mozilla.org/build/tools/file/726a439dc568/scripts/tooltool/fetch_and_unpack.sh

See bug 775509, attachment 643883 [details] [diff] [review] and http://hg.mozilla.org/build/tools/file/726a439dc568/scripts/tooltool/fetch_and_unpack.sh.
Attachment #644034 - Flags: review-
(In reply to Rail Aliiev [:rail] from comment #3)
> You don't need to define tooltool_manifest_src per branch anymore. Define
> those per platform in PLATFORM_VARS and you are done.

What is the mechanism that ensures only comm-central and try-comm-central will receive those changes?
mconley:

Unfortunately even with the attached patches, the tree will need work before it compiles cleanly:

 http://people.mozilla.org/~jhopkins/bug775670/buildlog.html

Any idea how much work to get a clean build?  Should we land the clang changes or instead find a way to continue to build with gcc?
(In reply to John Hopkins (:jhopkins) from comment #5)
> (In reply to Rail Aliiev [:rail] from comment #3)
> > You don't need to define tooltool_manifest_src per branch anymore. Define
> > those per platform in PLATFORM_VARS and you are done.
> 
> What is the mechanism that ensures only comm-central and try-comm-central
> will receive those changes?

The wrapper script runs noop if there is no manifest in the source tree:
http://hg.mozilla.org/build/tools/file/726a439dc568/scripts/tooltool/fetch_and_unpack.sh#l10
Comment on attachment 644043 [details] [diff] [review]
comm-central patch

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

::: mail/config/tooltool-manifests/linux32/clang.manifest
@@ +1,3 @@
> +[
> +{
> +"clang_version": "r160364"

So here you are binding mail to a specific clang version, which means you will need to update it manually every time Firefox does so. Will the Firefox team be aware of this and also change mail configs, or can we find a way to use the Firefox configs?
Getting it to build is pretty easy, I did this recently when trying to uncrash my build. 

The only thing I didn't investigate is getting the mdimporter to work, which is why its commented out. I believe the problem was that the mdimporter is compiled with Xcode, which doesn't detect the SVN clang as a valid version.
From the comm-central build log:

/builds/slave/tb-c-cen-osx64/build/mailnews/compose/src/nsMsgAppleEncode.cpp:109:44: error: invalid suffix on literal; C++11 requires a space between literal and identifier [-Wreserved-user-defined-literal]
clang: warning: argument unused during compilation: '-I ../../../../../mailnews/addrbook/src'
                                                   
/builds/slave/tb-c-cen-osx64/build/mailnews/compose/src/nsMsgAppleEncode.cpp:350:17: error: invalid suffix on literal; C++11 requires a space between literal and identifier [-Wreserved-user-defined-literal]
                                                CRLF"--%s"CRLF, 
                                                          ^

Curious that this doesn't pop up when I build locally.
(In reply to Mike Conley (:mconley) from comment #11)
> From the comm-central build log:
> 
> 
> Curious that this doesn't pop up when I build locally.

(with clang) - though perhaps the newer version is more strict about this sort of thing.
address rail's feedback.  remove a few unused platform configs rather than update them.
Attachment #644034 - Attachment is obsolete: true
Attachment #644307 - Flags: review?(rail)
Attachment #644043 - Flags: review?(rail)
Attachment #644055 - Flags: review?(rail)
Raphael:

Hey - any insight on what we're hitting and why?

-Mike
I spoke with mconley in IRC and he has given the ok to apply the clang config changes.
Attachment #644307 - Flags: review?(rail) → review+
Comment on attachment 644055 [details] [diff] [review]
mozilla-central patch

I'd like to hear Rafael's thoughts about this way of "detecting" the proper location of clang.
Attachment #644055 - Flags: review?(rail) → review?(respindola)
(In reply to Mike Conley (:mconley) from comment #14)
> Raphael:
> 
> Hey - any insight on what we're hitting and why?

The "requires a space between literal and identifier" error? That is probably because of different versions of clang and c++11 is a recent addition.

> -Mike

Cheers,
Rafael
Comment on attachment 644055 [details] [diff] [review]
mozilla-central patch

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

It is ok with me if you add a comment saying which cases each side of the 'if' is covering (regular tree or comm-central). Just wait for rail to comment too in case he knows a better way.
Attachment #644055 - Flags: review?(respindola)
Attachment #644055 - Flags: review+
Attachment #644055 - Flags: feedback?(rail)
Ok, thanks Raphael. This patch cleared up the build problems for me locally.
(In reply to Mike Conley (:mconley) from comment #19)
> Created attachment 644340 [details] [diff] [review]
> comm-central: Put spaces between literals and identifiers
> 
> Ok, thanks Raphael. This patch cleared up the build problems for me locally.

I think that is the correct thing to do, but you should probably realign the comments in mailnews/import/eudora/src/nsEudoraFilters.cpp :-)
Comment on attachment 644055 [details] [diff] [review]
mozilla-central patch

jhopkins, could you add comments for "if" and "elif" when you land?
Attachment #644055 - Flags: feedback?(rail) → feedback+
Comment on attachment 644043 [details] [diff] [review]
comm-central patch

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

::: build/macosx/common
@@ +3,5 @@
> +    export CXX=$topsrcdir/clang/bin/clang++
> +elif [ -d "$topsrcdir/../clang" ]; then
> +    export CC=$topsrcdir/../clang/bin/clang
> +    export CXX=$topsrcdir/../clang/bin/clang++
> +fi

A nit. Could you transplant the comments from the mozilla-central patch here as well.
Attachment #644043 - Flags: review?(rail) → review+
Attachment #644342 - Flags: review+
Comment on attachment 644342 [details] [diff] [review]
comm-central: Put spaces between literals and identifiers (v2)

Landed in http://hg.mozilla.org/comm-central/rev/ec138f1fcabf
Attachment #644342 - Flags: checked-in+
Builds are green and tree is reopened.  Thanks, everyone!
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
(In reply to Philipp Kewisch [:Fallen] from comment #10)
> The only thing I didn't investigate is getting the mdimporter to work, which
> is why its commented out. I believe the problem was that the mdimporter is
> compiled with Xcode, which doesn't detect the SVN clang as a valid version.
Do you had an error for mdimporter which sayed "failed to exec real xcrun"? I had this in Bug 764964 and fixed it with xcode-select -switch.
Blocks: 778632
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.