Closed Bug 1431113 Opened 3 years ago Closed 2 years ago

Remove #ifdef DEBUG_jp96085

Categories

(NSS :: Libraries, enhancement, P3)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ttaubert, Assigned: manishkk)

Details

(Keywords: good-first-bug)

Attachments

(1 file, 1 obsolete file)

We should get rid of "DEBUG_jp96085".
Hey I would like to work on this bug. Do you want to remove all instances of "DEBUG_jp96085" or just the line "#ifdef DEBUG_jp96085" in the file secport.c
Hey Aaron, glad you want to help! Let's remove all occurrences of "DEBUG_jp96085" in NSS.
Attached patch 1431113.patch (obsolete) — Splinter Review
Sorry for the late reply. I have made a patch for this bug. Can you check it out?
Flags: needinfo?(ttaubert)
Attachment #8958945 - Flags: review+
Comment on attachment 8958945 [details] [diff] [review]
1431113.patch

Thanks Aaron, please set review?(name) next time :) I'll take a look!
Flags: needinfo?(ttaubert)
Attachment #8958945 - Flags: review+ → review?(ttaubert)
Comment on attachment 8958945 [details] [diff] [review]
1431113.patch

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

That patch looks great. However, NSS is developed separately, as its own library. Can you please do the following:

1) Clone https://hg.mozilla.org/projects/nss
2) Apply your patch locally
3) Check via ./build.sh that NSS still builds

And then re-upload your patch against the NSS repository and ask me for review again?

Thanks!
Attachment #8958945 - Flags: review?(ttaubert)
Priority: -- → P3
seems no one is working on this. can I assign this bug to myself?

only I need to remove occurrence of 'DEBUG_jp96085' from these files?
https://dxr.mozilla.org/mozilla-central/search?q=DEBUG_jp96085&redirect=false
Flags: needinfo?(ttaubert)
(In reply to Manish Kumar from comment #6)
> seems no one is working on this. can I assign this bug to myself?

Sure!

> only I need to remove occurrence of 'DEBUG_jp96085' from these files?
> https://dxr.mozilla.org/mozilla-central/search?q=DEBUG_jp96085&redirect=false

Yep, just take a look at the previous comments.
Flags: needinfo?(ttaubert)
Assignee: nobody → 1991manish.kumar
Hi :fkiefer or :franziskus

can u tell bit about these 3 steps of NSS?
this should be inside the mercurial repo?

(In reply to Tim Taubert [:ttaubert] (inactive) from comment #5)
> Comment on attachment 8958945 [details] [diff] [review]
> 1431113.patch
> 
> Review of attachment 8958945 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That patch looks great. However, NSS is developed separately, as its own
> library. Can you please do the following:
> 
> 1) Clone https://hg.mozilla.org/projects/nss
> 2) Apply your patch locally
> 3) Check via ./build.sh that NSS still builds
>
Flags: needinfo?(franziskuskiefer)
Sorry for the delay.

> can u tell bit about these 3 steps of NSS?
> this should be inside the mercurial repo?

Yes, NSS in the linked mercurial repository. Your patch is against the Firefox repository that only holds a copy of NSS.
You have to check it out and make your changes there, and then upload that patch.
Flags: needinfo?(franziskuskiefer)
For this:
https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/util/secport.c#202

What to remove just line#202 or whole function?
Flags: needinfo?(franziskuskiefer)
No, you can't just remove the one line or you'll have an unmatched #endif, and we don't want the debugging code in a regular build. Given:

  #ifdef DEBUG_jp96085
     ... stuff ...
  #endif

We'd want to remove the entire chunk of personal debugging cruft. If there were a #else or #elif in there you'd have to untangle the debug code from the real code, but I didn't see any like that in the DEBUG_jp... cases.

There are 3 instances in security/nss/cmd/crlutil/crlutil.c as well, btw.
Attached patch Patch_bug1431113Splinter Review
I followed these steps.
https://bugzilla.mozilla.org/show_bug.cgi?id=1431113#c5

Please review the patch!
Attachment #8958945 - Attachment is obsolete: true
Attachment #9005467 - Flags: review?(franziskuskiefer)
Comment on attachment 9005467 [details] [diff] [review]
Patch_bug1431113

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

Looks good to me.

(Franziskus is out on leave for several weeks, so I stole this shamelessly.)
Attachment #9005467 - Flags: review?(franziskuskiefer) → review+
Flags: needinfo?(franziskuskiefer)
:jcj

Thanks! Even I was holding this bug from last 5 months and This is mine first bug related to Security/NSS
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.