479978 - remove references to MOZ_PROFILE from the build system

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

I removed MOZ_PROFILE support a while ago, since it essentially duplicated the functionality of MOZ_DEBUG_SYMBOLS. Looks like I missed a few spots though:

http://hg.mozilla.org/mozilla-central/annotate/920e3f7dfd49//configure.in#l8023 (I wouldn't actually click this link, it takes ages to load.)
http://hg.mozilla.org/mozilla-central/annotate/69c86f3acc5a/Makefile.in#l137
http://hg.mozilla.org/mozilla-central/annotate/69c86f3acc5a/config/autoconf.mk.in#l592
http://hg.mozilla.org/mozilla-central/annotate/69c86f3acc5a/config/config.mk#l246

We should clean up the rest of those. CCing a few of my favorite code janitors.

Comment 1

[Serge Gautherie (:sgautherie)]

(In reply to comment #0)
> Looks like I missed a few spots though:

m-c:
http://mxr.mozilla.org/mozilla-central/search?string=MOZ_PROFILE%5B%5EL_%5D&regexp=on&case=on
Found 17 matching lines in 11 files

c-c (+ m-1.9.1):
http://mxr.mozilla.org/comm-central/search?string=MOZ_PROFILE%5B%5EL_%5D&regexp=on&case=on&find=%2Fmozilla%2F
Found 17 matching lines in 11 files
http://mxr.mozilla.org/comm-central/search?string=MOZ_PROFILE%5B%5EL_%5D&regexp=on&case=on
Found 25 matching lines in 17 files

Comment 2

[Serge Gautherie (:sgautherie)]

(In reply to comment #0)
> http://hg.mozilla.org/mozilla-central/annotate/69c86f3acc5a/config/config.mk#l246

Should this 'if' block be removed or the comment updated (to what ?) only ?

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Av1-CC) 2 of the files

(untested but trivial)

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1-CC) c-sdk part [Checkin: Comment 28]

(Untested and I don't know if you want this (yet),
but, if you do, here's the patch.)

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to comment #2)
> Should this 'if' block be removed or the comment updated (to what ?) only ?

Iirc, Ted told me that removing just the comment line would do (for now).

Comment 6

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a-CC) the 3 files

Av1-CC, with comment 5 suggestion(s).

Comment 7

[Mark Smith [:mcs]]

The directory/c-sdk changes look OK, but I am hesitant to remove the MOZ_PROFILE support in case someone is using it.  I added Rich Megginson and Anton Bobrov to the bug CC so they can comment.

Comment 8

[Anton Bobrov]

i think it should be safe to remove. i'm not aware of anybody using it and
also cant imagine why anybody would need MS C 7.0 compatible debug format.

Comment 9

[Rich Megginson]

Looks fine.

Comment 10

[Robert Kaiser]

Comment on attachment 365414 [details] [diff] [review]
(Av1a-CC) the 3 files

>diff --git a/config/config.mk b/config/config.mk
>-# MOZ_PROFILE equivs for win32
> ifeq ($(OS_ARCH)_$(GNU_CC),WINNT_)
> ifdef MOZ_DEBUG
> ifneq (,$(MOZ_BROWSE_INFO)$(MOZ_BSCFILE))
> OS_CFLAGS += -FR
> OS_CXXFLAGS += -FR
> endif
> else # ! MOZ_DEBUG

I'm somewhat unsure about this hunk, it looks wrong to me to remove a comment and leave the block it annotates completely undescribed after that. if this block is just an equivalent of stuff that MOZ_PROFILE does elsewhere, doesn't it mean that this whole block can be removed? (not that i understand what it does) If not, shouldn't the comment be corrected so that it still explains what the block is doing?

r- just for that unclarity, the rest looks fine.

Please also try to do the mozilla part though, it surely helps to clean up that old cruft.

Comment 11

[(not currently active) Ted Mielczarek]

(In reply to comment #10)
> I'm somewhat unsure about this hunk, it looks wrong to me to remove a comment
> and leave the block it annotates completely undescribed after that. if this
> block is just an equivalent of stuff that MOZ_PROFILE does elsewhere, doesn't
> it mean that this whole block can be removed? (not that i understand what it
> does) If not, shouldn't the comment be corrected so that it still explains what
> the block is doing?

Yeah, I was unsure about this as well. From a brief inspection, the block is clearly not doing the same thing as MOZ_PROFILE, it relates to creating a "browse info file" from VC++, which is the source code indexing that the IDE uses. However, I didn't dig into it enough to figure out what a good comment would be.

Comment 12

[Serge Gautherie (:sgautherie)]

(In reply to comment #10)
> shouldn't the comment be corrected so that it still explains what
> the block is doing?

My (implicit) proposal is to remove the (bogus !?) comment here,
then file a followup bug to restore a meaningful comment.

Comment 13

[Serge Gautherie (:sgautherie)]

(In reply to comment #12)
> My (implicit) proposal is to remove the (bogus !?) comment here,
> then file a followup bug to restore a meaningful comment.

Or the other way round: leave the comment and file a bug to update it... if you prefer.

Comment 14

[Mark Smith [:mcs]]

Comment on attachment 365364 [details] [diff] [review]
(Bv1-CC) c-sdk part
[Checkin: Comment 28]

OK, it looks like the directory/c-sdk patch is good to go then.

Comment 15

[Robert Kaiser]

(In reply to comment #13)
> (In reply to comment #12)
> > My (implicit) proposal is to remove the (bogus !?) comment here,
> > then file a followup bug to restore a meaningful comment.
> 
> Or the other way round: leave the comment and file a bug to update it... if you
> prefer.

Either that or change it to point to comment #11 of this bug or the new bug you filed. In any case, let's make sure that this block doesn't get less understandable than it already is. If even Ted doesn't know and he knows significantly more Windows build stuff than me, there's surely some need to have a comment there, ideally an improved one.
You actually could change to comment to "XXX: What does this? bug 4..... filed for better explanation" and make that new bug reference the discussion in here, so someone looking at the code can actually find out _something_ about it.

Comment 16

[Serge Gautherie (:sgautherie)]

Attachment: (Av1b-CC) the 3 files [Checkin: Comment 21]

Av1a-CC, with comment 15 suggestion(s).

(In reply to comment #15)
> You actually could change to comment to "XXX: What does this? bug 4..... filed
> for better explanation" and make that new bug reference the discussion in here,

I filed bug 482434.

Comment 17

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1-MC) the 9 files [Checkin: See comment 30]

Comment 18

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1-MC) nsprpub part [Checkin: Comment 27]

Comment 19

[Robert Kaiser]

Comment on attachment 366533 [details] [diff] [review]
(Av1b-CC) the 3 files
[Checkin: Comment 21]

looks good to me with that comment change, thanks.

Comment 20

[Wan-Teh Chang]

Comment on attachment 366538 [details] [diff] [review]
(Dv1-MC) nsprpub part [Checkin: Comment 27]

r=wtc.  Thanks.

Comment 21

[Serge Gautherie (:sgautherie)]

Comment on attachment 366533 [details] [diff] [review]
(Av1b-CC) the 3 files
[Checkin: Comment 21]


http://hg.mozilla.org/comm-central/rev/aa90f45fce46

Comment 22

[(not currently active) Ted Mielczarek]

Comment on attachment 366536 [details] [diff] [review]
(Cv1-MC) the 9 files
[Checkin: See comment 30]

-ifdef MOZ_PROFILE
-	echo splitting symbols out of binaries

Just remove this whole splitsymbols target while you're here, it's no longer used. (Remove it from the deliver: target below as well. Actually, I think you can kill deliver and all its children, I'm fairly certain they're not used anywhere, it used to be called from Tinderbox.)

diff --git a/security/coreconf/WIN32.mk b/security/coreconf/WIN32.mk
--- a/security/coreconf/WIN32.mk
+++ b/security/coreconf/WIN32.mk

This needs to be in a separate bug (or at least patch), since it's NSS and I'm not a peer there.

Comment 23

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1-MC) security part

(split from Dv1-MC)

Comment 24

[Serge Gautherie (:sgautherie)]

Attachment: (Fv1-CC) Remove |splitsymbols| target call [Checkin: Comment 29]

Per comment 22.

Comment 25

[Serge Gautherie (:sgautherie)]

(In reply to comment #22)
> Just remove this whole splitsymbols target while you're here, it's no longer
> used. (Remove it from the deliver: target below as well.

I'll do.

> Actually, I think you can kill deliver and all its children

I prefer to leave this to bug 484799 and bug 484818.

> This needs to be in a separate bug (or at least patch)

Done: patch Ev1-MC.

Comment 26

[Wan-Teh Chang]

Attachment: (Ev2-MC) security part by Serge Gautherie [Checkin: Comment 26]

r=wtc.

The comments should also be removed.  I also merged
two ifdefs that became the same after MOZ_PROFILE was
removed.

I checked this in on the NSS trunk (NSS 3.12.3).

Checking in WIN32.mk;
/cvsroot/mozilla/security/coreconf/WIN32.mk,v  <--  WIN32.mk
new revision: 1.32; previous revision: 1.31
done

Comment 27

[Wan-Teh Chang]

Comment on attachment 366538 [details] [diff] [review]
(Dv1-MC) nsprpub part [Checkin: Comment 27]

I checked this in on the NSPR trunk (NSPR 4.8).

Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.256; previous revision: 1.255
done
Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.251; previous revision: 1.250
done

Comment 28

[Mark Smith [:mcs]]

I committed the LDAP C SDK portion (Bv1-CC)to the CVS trunk:

mozilla/directory/c-sdk/config/WIN32.mk
  new revision: 5.4; previous revision: 5.3
mozilla/directory/c-sdk/configure
  new revision: 5.73; previous revision: 5.72
mozilla/directory/c-sdk/configure.in
  new revision: 5.67; previous revision: 5.66

Comment 29

[Serge Gautherie (:sgautherie)]

Comment on attachment 368911 [details] [diff] [review]
(Fv1-CC) Remove |splitsymbols| target call
[Checkin: Comment 29]


http://hg.mozilla.org/comm-central/rev/688fec4c59ca

Comment 30

[Serge Gautherie (:sgautherie)]

Comment on attachment 366536 [details] [diff] [review]
(Cv1-MC) the 9 files
[Checkin: See comment 30]


http://hg.mozilla.org/mozilla-central/rev/228e3145c1a6
Cv1-MC, with comment 22 suggestion(s).

Comment 31

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 366536 [details] [diff] [review]
(Cv1-MC) the 9 files
[Checkin: See comment 30]

Hard to tell what parts of these patches we would need to take on the branch, and doesn't really feel like it's needed. Thanks for offering, though!

Comment 32

[Serge Gautherie (:sgautherie)]

Comment on attachment 366536 [details] [diff] [review]
(Cv1-MC) the 9 files
[Checkin: See comment 30]


(In reply to comment #31)
> Hard to tell what parts of these patches we would need to take on the branch,

This part (only)!