Last Comment Bug 455348 - Change hyphens to underscores in DEBUG_$(shell whoami).
: Change hyphens to underscores in DEBUG_$(shell whoami).
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Build (show other bugs)
: unspecified
: All All
: -- minor (vote)
: 3.12.2
Assigned To: Wan-Teh Chang
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-15 10:27 PDT by Wan-Teh Chang
Modified: 2008-10-03 12:15 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
remove DEBUG_nelsonb from NSS sources (checked in) (2.40 KB, patch)
2008-09-24 19:35 PDT, Nelson Bolyard (seldom reads bugmail)
wtc: review+
Details | Diff | Review
change to my current userid (1.02 KB, patch)
2008-09-25 17:35 PDT, Julien Pierre
nelson: review+
Details | Diff | Review
Proposed patch: change hyphens to underscores (930 bytes, patch)
2008-09-25 18:11 PDT, Wan-Teh Chang
julien.pierre: review+
Details | Diff | Review

Description Wan-Teh Chang 2008-09-15 10:27:48 PDT
I suggest that the coreconf build system not define DEBUG_$(shell whoami)
(or its equivalent for platforms that don't have the 'whoami' command),
and we remove DEBUG_userid from the NSS source tree.

The reason is that we need to process the user id if it contains
characters that are illegal in a macro name, such as spaces and
hyphens.  On my home computer, my user id is "Wan-Teh Chang", so
I need to substitute the '-' and ' ' by an underscore '_', or
patch coreconf to not define the macro.

I never use DEBUG_wtc during development, and don't think
such conditionally compiled code should be checked in.

The NSS source tree contains these DEBUG_userid macros:
DEBUG_kaie
DEBUG_nb95248
DEBUG_jp96085
DEBUG_nelsonb
DEBUG_volvov
DEBUG_volkov1
DEBUG_stevep
DEBUG_jpierre

The PSM source tree contains these DEBUG_userid macros:
DEBUG_javi
DEBUG_jgmyers
Comment 1 Nelson Bolyard (seldom reads bugmail) 2008-09-15 14:41:16 PDT
Wan-Teh, 
My userid is not nelsonb on most of the computers on which I build, but
I have a script that sets a number of environment variables that I commonly
use when I build, and one of the variables it sets is USERNAME=nelsonb

This feature has a long history, and as you can see, many people have used it.
We can see who will be inconvenienced by removing it.
I wonder: who (or what) benefits from removing it?
Comment 2 Wan-Teh Chang 2008-09-15 14:59:11 PDT
I benefit from removing it.  If any of you really needs it, I'll just
submit a patch to substitute hyphens by underscores.
Comment 3 Nelson Bolyard (seldom reads bugmail) 2008-09-15 19:34:05 PDT
Speaking for myself only, #ifdef DEBUG_nelsonb appears in only 3 places in NSS.
Two of them can be deleted.  One can be converted into simple ifdef DEBUG, I
think.

In sslsnce.c, these lines can be deleted:

1182 #if defined(DEBUG_nelsonb)
1183     printf("sizeof(sidCacheEntry) == %u\n", sizeof(sidCacheEntry));
1184 #endif

In selfserv.c, these lines can be deleted:

2064 #ifdef DEBUG_nelsonb
2065         WaitForDebugger();
2066 #endif

and the following #ifdef can be converted to #ifdef DEBUG

1769 #ifdef DEBUG_nelsonb
1770 
1771 #if defined(XP_UNIX) || defined(XP_OS2) || defined(XP_BEOS)
1772 #define SSL_GETPID getpid
1773 #elif defined(_WIN32_WCE)
1774 #define SSL_GETPID GetCurrentProcessId
1775 #elif defined(WIN32)
1776 extern int __cdecl _getpid(void);
1777 #define SSL_GETPID _getpid
1778 #else
1779 #define SSL_GETPID() 0   
1780 #endif
...

and the function WaitForDebugger can be deleted.
Comment 4 Nelson Bolyard (seldom reads bugmail) 2008-09-24 19:35:19 PDT
Created attachment 340279 [details] [diff] [review]
remove DEBUG_nelsonb from NSS sources (checked in)

There was a time when I valued these things, but not any more.  
So, I'm ridding NSS of them.  Wan-Teh, please review.
Comment 5 Wan-Teh Chang 2008-09-25 09:52:49 PDT
Comment on attachment 340279 [details] [diff] [review]
remove DEBUG_nelsonb from NSS sources (checked in)

r=wtc.
Comment 6 Julien Pierre 2008-09-25 17:35:31 PDT
Created attachment 340476 [details] [diff] [review]
change to my current userid

I would like to keep this, as well as the assertion code in secport.c that's in an ifdef block under my name.
Comment 7 Nelson Bolyard (seldom reads bugmail) 2008-09-25 17:55:48 PDT
Wan-Teh, does changing DEBUG_$(shell whoami) to DEBUG_${USERNAME} in that
makefile help you?
Comment 8 Wan-Teh Chang 2008-09-25 18:11:19 PDT
Created attachment 340483 [details] [diff] [review]
Proposed patch: change hyphens to underscores

The solution is to change characters that are illegal in a macro name
to underscores.  On Unix, usernames can't contain spaces, so we only
need to change hyphens.
Comment 9 Wan-Teh Chang 2008-09-25 18:16:01 PDT
Comment on attachment 340483 [details] [diff] [review]
Proposed patch: change hyphens to underscores

Nelson, switching to ${USERNAME} won't help.

I didn't realize USERNAME is an environment variable on Unix.
Should the variable in my patch be renamed USERID to avoid
confusion?
Comment 10 Julien Pierre 2008-09-25 18:47:38 PDT
Wan-Teh,

USERNAME is not an environment variable on my Solaris box. USER is . USERID is not . I don't know if it's standardized .
Comment 11 Nelson Bolyard (seldom reads bugmail) 2008-09-28 14:33:52 PDT
Comment on attachment 340476 [details] [diff] [review]
change to my current userid

r=nelson
Comment 12 Nelson Bolyard (seldom reads bugmail) 2008-09-29 20:54:05 PDT
Comment on attachment 340279 [details] [diff] [review]
remove DEBUG_nelsonb from NSS sources (checked in)

Checking in cmd/selfserv/selfserv.c; new: 1.88; previous: 1.87
Checking in lib/ssl/sslsnce.c;       new: 1.46; previous: 1.45
Comment 13 Julien Pierre 2008-10-01 21:11:48 PDT
Thanks for the review, Nelson.

Checking in nss/cmd/crlutil/crlutil.c;
/cvsroot/mozilla/security/nss/cmd/crlutil/crlutil.c,v  <--  crlutil.c
new revision: 1.31; previous revision: 1.30
done

Can we mark this bug fixed ?
Comment 14 Wan-Teh Chang 2008-10-03 12:15:55 PDT
I checked in my patch (attachment 340483 [details] [diff] [review]) on the NSS trunk (NSS 3.12.2).

Checking in UNIX.mk;
/cvsroot/mozilla/security/coreconf/UNIX.mk,v  <--  UNIX.mk
new revision: 1.6; previous revision: 1.5
done

I changed the summary of the bug report to reflect what we decided to do.

Note You need to log in before you can comment on or make changes to this bug.