261232 - Require builders to specify application(s) to client.mk and configure when building

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, enhancement)

Description

[Benjamin Smedberg]

Since Firefox has become a mass-market phenomenon, we have lots more people
trying to build it. The unix-familiar ones download the source, run configure,
and expect it to "just work", and it doesn't because of our slightly wacky
MOZ_PHOENIX build system and the configure options in browser/config/mozconfig.

So I want to require builders to specify an application to build
--enable-application=suite|browser|mail|composer|calendar|xulrunner

I would prefer to use generic (non-branded) names instead of
firefox/thunderbird/nvu/sunbird in case the brand names change (again). This
would also allow me to get rid of the app-specific mozconfig and do some basic
sanity-checking of the build options people are trying to use.

Comment 1

[Darin Fisher]

sounds like a good idea to me.

Comment 2

[Benjamin Smedberg]

Attachment: part 1: Kick client.mk in the 'nads

Comment 3

[Benjamin Smedberg]

Comment on attachment 161400 [details] [diff] [review]
part 1: Kick client.mk in the 'nads

This will of course require ample notification on the newsgroups and updating
tinderboxen, etc.

Comment 4

[cls]

Comment on attachment 161400 [details] [diff] [review]
part 1: Kick client.mk in the 'nads

Looks good but I have a few nits:

The directory name is 'libart_lgpl'.  You used libart-lgpl in a few places.  

If you're going to warn about MOZ_INTERNAL_LIBART_LGPL, you might as well just
make it a hard error.  If you have MOZ_INTERNAL_LIBART_LGPL set but not
MOZ_CO_PROJECT or MOZ_CO_MODULE, it will process with the very limited pull
rather than spitting out the intended error.

You should probably have a similar error message when the other project
specific env variables are set (MOZ_PHOENIX, MOZ_THUNDERBIRD, etc).

Likewise when MOZ_CO_PROJECTS contains an unknown (or undefined) project.

IIRC, some shells try to interpret ab-DE as an arithmetic expression.  The
MOZ_CO_LOCALES example should keep the quotes.

Comment 5

[Benjamin Smedberg]

Attachment: client.mk + configure changes

OK, to get rid of the MOZ_PHOENIX/MOZ_INTERNAL_* I can't do it incrementally,
so here's the entire shebang.

Comment 6

[Justin Wood (:Callek)]

bsmedberg: correct me if I am wrong but on your updated patch, ("client.mk +
configure changes"), does not that cause Sunbird _not_ to build correctly...

MOZ_CALENDAR is no longer defined because you remove the --enable-calendar from
the sunbird mozconfig, and if --enable-calendar is not defined it over-rides
MOZ_CALENDAR from the app-selection... 
(https://bugzilla.mozilla.org/attachment.cgi?id=161799&action=diff#mozilla/configure.in_sec5)
is where it would be undefined.

As a further nit (not my field but still a nit)
https://bugzilla.mozilla.org/attachment.cgi?id=161799&action=diff#mozilla/xpfe/bootstrap/nsAppRunner.cpp_sec1
Do we really want to hardcode MOZ_APP_NAME in nsAppRunner.cpp?

Thanks for listening (adding mostafah to CC since he is resident calendar owner)

Comment 7

[Benjamin Smedberg]

Callek, you are thankfully wrong ;)  the third argument to the
MOZ_ARG_ENABLE_BOOL macro is not used if you don't specify --enable-calendar,
it's used if you specify --disable-calendar; try it, you'll like it.

And yes, I did want to hardcode "mozilla" in xpfe/bootstrap/nsAppRunner.cpp...
nobody except seamonkey is be using this file, or should be.

Comment 8

[cls]

Comment on attachment 161799 [details] [diff] [review]
client.mk + configure changes

> CYGWIN_WRAPPER=
> MOZ_USER_DIR=".mozilla"
>-MOZ_APP_NAME=mozilla

>+case "$MOZ_BUILD_APP" in

>+standalone)
>+  ;;

MOZ_APP_NAME should always be set.  In the standalone case, it's not set and
that'll break the handful of places that use it in a standalone build (like
build/unix/Makefile.in & 'make install'). Same for MOZ_APP_VERSION.

> if test "$MOZ_SVG_RENDERER_LIBART"; then

>+  dnl libart's configure hasn't been run yet, but we know what the
>+  dnl answer should be anyway
>+  MOZ_LIBART_CFLAGS='-I${DIST}/include/libart_lgpl'
>+  case "$target_os" in
>+  msvc*|mks*|cygwin*|mingw*)
>+      MOZ_LIBART_LIBS='$(DIST)/lib/$(LIB_PREFIX)moz_art_lgpl.$(IMPORT_LIB_SUFFIX)' 
>+      ;;
>+  beos*)
>+      MOZ_LIBART_LIBS='-lmoz_art_lgpl -lroot -lbe'
>+      ;;
>+  *)
>+      MOZ_LIBART_LIBS='-lmoz_art_lgpl -lm'
>+      ;;
>+  esac
>+  AC_FUNC_ALLOCA

I'm assuming that we're still not pulling libart by default for historical
licensing reasons.  Since we're not pulling it by default, we need to
explicitly check for the directory here and make it clear on how to pull the
tree.

There are also MOZ_INTERNAL_LIBART_LGPL references in /minimo.mk & /Makefile.in
.

Comment 9

[Benjamin Smedberg]

Attachment: client.mk + configure changes, rev 2.1

Comment 10

[Benjamin Smedberg]

Chris, I am not going to mess with minimo.mk since it is a bad fork of client.mk
and I don't actually break it (it still works the way it always did).

Comment 11

[Darin Fisher]

FWIW, the minimo-arm tinderbox doesn't even pull minimo.mk.  That may be a
better way to build minimo, but it isn't required.  Maybe ask dougt ;-)

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

please also update the standalone xpcom build instructions at
http://www.mozilla.org/projects/xpcom/xpcom-standalone.html - thanks!

Comment 13

[cls]

Comment on attachment 166185 [details] [diff] [review]
client.mk + configure changes, rev 2.1

+  if ! -f $topsrcdir/other-licenses/libart_lgpl/Makefile.in; then

That should be 'if test !'

And you still missed the MOZ_LIBART_INTERNAL_LGPL ifdef in mozilla/Makefile.in
which causes the build to fail in layout/svg .

Fix those && r=cls

Comment 14

[Alex Vincent [:WeirdAl]]

Comment on attachment 166185 [details] [diff] [review]
client.mk + configure changes, rev 2.1

One little question:
>+composer)
>+  MOZ_APP_NAME=nvu

Have we actually landed N|Vu source code in CVS?  There may be an unintended
slight to Disruptive Innovations or nvudev.org if we use their app name for
Mozilla code (not code ported back from N|vu).

Comment 15

[Benjamin Smedberg]

AJ, 
1) NVU is already there, I just moved the definition around
2) glazou and I work together pretty well, I think he would let me know if there
were problems.

Comment 16

[Axel Hecht]

I take it that this patch fixes the toolkit checkout for standalone apps, too?
Bug 253270.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Making MOZ_MAPINFO cause an error seems like a bad idea, since client.mk wasn't
the only thing that was checking the environment variable.  It also wasn't a
good thing to do without updating the tinderboxes *first*.

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

note that it's the tinderbox script that sets MOZ_MAPINFO:
http://lxr.mozilla.org/mozilla/source/tools/tinderbox/build-seamonkey-util.pl#403

Comment 19

[Benjamin Smedberg]

I did go through the tinderboxen mozconfig, I didn't realize that MOZ_MAPINFO
was being set from the tinderbox script directly. I can make it a warning
temporarily until the tinderbox scripts are updated.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Have all the tinderboxes that are setting MOZ_CO_MODULE (with SeaMonkeyAll in
it), MOZ_INTERAL_LIBART_LGPL, etc., been updated?  If not, this should be backed
out until they are.

I updated the tinderbox script to pull client.mk every time.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

balsa is still setting MOZ_CO_MODULE the old way in both its SeaMonkey-Ports and
Firefox trees.  So is brad. There are probably some others floating around doing
the same thing.  (I like the semantics change, though, and I guess that one's
probably not too harmful, since it's one command line.)

Aren't we going to run into tinderbox problems for the tinderboxes that build
SVG as well?

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And dare I say that this should probably not have gone in the first day after
the tree reopened when everybody else wanted to get things in?

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I've also updated build-seamonkey-util.pl to set MOZ_CO_MODULE instead of
MOZ_MAPINFO.

I've updated:
balsa (script and config)
luna (script)
myotonic (script)

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I've updated:

monkeypox (scripts and config)

I may do some more later...

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Updated:

myotonic (mozconfig)
bismark (mozconfig and scripts)

these two weren't even updated with the two lines to build the suite.  That says
to me that this wasn't ready to land, so I'm stopping now.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fixed: speedracer worms otaku binus imola boxset gruff

Next time you do this (land something requiring tinderbox changes before the
tinderboxes are updated) I'm going to back you out.  I really should have done
that today.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(The reason I didn't do that today was actually that you broke things badly
enough that backing you out wouldn't have been enough to fix the bustage.)

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(The reason I didn't do that today was actually that you broke things badly
enough that backing you out wouldn't have been enough to fix the bustage,
although I've now fixed the tinderbox scripts so that it would have been.)

Comment 29

[Frederic Bezies]

Sorry for """spamming""" the bug but it looks like there is some CVS checkout
problems...

I grabbed using CVS a full blank new source adding in my .mozconfig these options :

mk_add_options MOZ_CO_PROJECT=suite
ac_add_options --enable-application=suite

Interesting part of cvsco.log :

"checkout start: mer nov 24 11:22:22 CET 2004
[...]
cvs checkout: warning: new-born mozilla/.cvsignore has disappeared
U mozilla/README/mozilla/README.build
cvs checkout: warning: new-born mozilla/README.txt has disappeared
cvs checkout: warning: new-born mozilla/client.mak has disappeared
cvs checkout: warning: new-born mozilla/configure has disappeared
cvs checkout: warning: new-born mozilla/configure.in has disappeared
cvs checkout: warning: new-born mozilla/allmakefiles.sh has disappeared
[...]
checkout finish: mer nov 24 11:34:13 CET 2004"

A work around is to use an older tarball and update it with CVS.

Sorry if this bug was already reported !

Comment 30

[Benjamin Smedberg]

Attachment: Workaround "cvs" bugs

This is apparently needed to work around bugs in some versions of the "cvs"
client, which cannot check out a Module and a directory-module at the same time
without getting confused.

Comment 31

[Benjamin Smedberg]

Comment on attachment 166927 [details] [diff] [review]
Workaround "cvs" bugs

Asking chase for review, but cls or other cvs-knowers are welcome to steal the
review.

Comment 32

[Frederic Bezies]

Sorry to "spam" this bug.

I applied this patch (patch -p0 < fix-newborn-cvs.patch), then I do a make -f
client.mk checkout to grab a blank new source.

And files in comment #29 are still missing :[

My CVS version is 1.11.17, under Fedora Core 3.

Comment 33

[Glenn Randers-Pehrson]

Re comment #29 -- I'm getting the same on FreeBSD, yesterday and today.  The
build eventually dies because it can't find configure.

Comment 34

[Glenn Randers-Pehrson]

My CVS version is 1.11.2.1 under FreeBSD 5.0.  I also tried
version 1.11.17 on a Fedora/Linux platform and 1.12.9 under SunOS 5.9
and all exhibited the same behaviour, new-born file "configure.in" disappeared.

Comment 35

[Jacek Piskozub]

Same result with Cygwin cvs 1.11.17 on Win32. The .../mozilla/ folder is empty
of all content except for client.mk and subfolders.

Comment 36

[Benjamin Smedberg]

Attachment: Final initial-checkout patch, committed

This fixes the problems with "new-born" files during initial checkout. It turns
out that there was a sticky tag leftover from the LDAP checkout step which was
not properly cleared when checking out the SeaMonkeyAll module. This patch
reorders the checkout steps so that the "directory" checkouts
(mozilla/accessible etc) are performed before SeaMonkeyAll. It was reviewed by
cmp and has been committed.

Comment 37

[Benjamin Smedberg]

Marking fixed. If you have further problems please file followup bugs.

Comment 38

[Glenn Randers-Pehrson]

Checkout from trunk is working OK now, thanks (FreeBSD, CVS 1.11.2.1)

Comment 39

[Johnny Stenback (:jst)]

So, um, with the current code if you simply type "make -f client.mk pull_all",
as you used to be able to in the good ol' days, we happily start pulling but we
only pull about 4 targets and then stop w/o any error or anything. That means
that those who pull just client.mk and use pull_all to pull the tree are left
with only a fraction of the source code and no error message what so ever.

Seems really wrong to be.

And shouldn't we have a good default (be it SeaMonkey or Firefox) that would be
pulled if you just pull w/o going through the trouble of creating a .mozconfig
file. Personally I've never had one in a source tree, I always create them in my
obj-dirs in which I build, and I've got lots of those for a given source tree.

Can we not add make targets for pulling our different apps in stead of requiring
creation of .mozconfig files?

/me is not at all happy with this so far :(

Comment 40

[Johnny Stenback (:jst)]

If one now follows the steps on http://www.mozilla.org/cvs.html one gets a
mozilla tree with the following directories:

build/  client.mk  CVS/  directory/  nsprpub/  security/

and no error, nothing.

Reopening, there's clearly more to be done here, and more docs need to be updated.

Comment 41

[Benjamin Smedberg]

Not-reporting the error is a regression from the last patch here, which added an
extra space to the module variable. That's now fixed, as is cvs.html.

Comment 42

[Jacek Piskozub]

cvs.html is not fixed. Yes, it's changed but now the checkout command is
described like this:
make -f client.mk checkout MOZ_CO_PROJECT=<option>

The cryptic reference to an <option> without explaining whatsoever what the
options might be is certain to confuse 90% of users (the remaining 10% are the
inhabitants of this bug and some Google Masters who will find the relevant BS
blog entry). Maybe some of the text from
http://www.saintpatrickdc.org/bsmedberg/index.php should be copied to cvs.html? 

Comment 43

[David G King]

These documentation pages also need to be updated :-

http://www.mozilla.org/build/win32.html
http://www.mozilla.org/build/mac.html

They are accessed from http://www.mozilla.org/download-mozilla.html

Comment 44

[Benjamin Smedberg]

cvs.html *is* fixed: Since several different applications are built from the
same basic source code, you must choose which application to checkout on the
command line using the MOZ_CO_PROJECT variable. The possible options are:

suite
    The traditional "Mozilla" seamonkey suite of browser, mail/news, composer,
and other applications. 
browser
    The standalone "Firefox" browser. 
mail
    The standalone "Thunderbird" mail/news client. 
composer
    The standalone HTML composer 
calendar
    The standalone "Sunbird" calendar app. 
xulrunner
    The next-generation XUL application launcher. 
macbrowser
    The "Camino" native browser for Macintosh. 

win32.html does not need to be updated, as it does not contain any checkout
instructions at all. And as I said, please file followup bugs, instead of
reopening this one endlessly.

Comment 45

[Jacek Piskozub]

Benjamin, do we live in alternate universes? I do not see this  very info on
cvs.html even as it is necessary to know how to pull any mozilla.org product.
You cannot mention an "<option>" without listing the actual possible values.

I am a scholar and I can assure you that if cvs.html were a scientific paper you
would not get any reviewer to allow publishing it as it is.

Comment 46

[Benjamin Smedberg]

Jacek, we must live in alternate universes, it's in the fifth paragraph.

Comment 47

[Michiel van Leeuwen (email: mvl+moz@)]

I think the misunderstanding is that the options are listed in the fifth
paragraph, but when you use the <option> notation a few paragraphs later, you
don't say that it is one of the options above. Yes, it is the same command as
above, but you have forgotten that once you are at the detailed instructions.

Comment 48

[Jacek Piskozub]

OK. I see it now. But I would not call that user friendly. It's not easy to see
the connection between the windy bullet list above and the <option> mentioned
below, especially when you generally know what you're doing but look only for
changes in the procedure.

Comment 49

[timeless]

Comment on attachment 166185 [details] [diff] [review]
client.mk + configure changes, rev 2.1

>Index: xpfe/bootstrap/nsAppRunner.cpp
>@@ -148,12 +148,14 @@
>+#define MOZ_APP_NAME "mozilla"

erm. there are companies that rebrand mozilla.exe, and seamonkey changed the name of the app too...

Comment 50

[Frank Wein [:mcsmurf]]

That one was removed again as part of Bug 285696 (suite rebranding).