Closed Bug 1480647 Opened 2 years ago Closed 2 years ago

Warn about header deprecation using #pragma message

Categories

(NSS :: Build, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ctruta, Assigned: ctruta)

Details

Attachments

(1 file, 3 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/67.0.3396.99 Safari/537.36

Steps to reproduce:

Both NSS and libsrtp have a header named "key.h", and in both projects, #include "key.h" is not qualified by an explicit path.

In our project, where both of these libraries are being used, we must be extra-careful with the include search paths, so that NSS and libsrtp don't step on each other's toes.

Since "key.h" in NSS is already deprecated, for the benefit of "keyhi.h", would it be acceptable to make things safer (and easier), by including "keyhi.h" directly in the NSS code, and also by replacing the plain comment with an actual custom warning (#pragma message)?
Attachment #8997255 - Flags: review?(rrelyea)
Comment on attachment 8997255 [details] [diff] [review]
Replace comment with pragma message; replace "key.h" with "keyhi.h"

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

r+ rrelyea.
We should probably put the pragma into keyt.h as well. in favor of keythi.h

bob
Attachment #8997255 - Flags: review?(rrelyea) → review+
Attachment #8997255 - Flags: checked-in?
(In reply to Robert Relyea from comment #2)
> We should probably put the pragma into keyt.h as well. in favor of keythi.h

Thanks for the review. Done. See the new bug-1480647-2.patch

I wasn't sure if you wanted keyt.h resolved in a separate bug, or in this one. Since the bug title is applicable to both, I decided to replace the old patch with the new one, here.

One thing I've done differently: patching the test nonspr10.c. I removed both key.h and keyt.h, because I didn't know how to shut up the pragma warning in a portable manner; but now, the availability of these headers is untested. I hope this kind of change is acceptable.

One last thing: I set the "checked-in?" flag. Is this the correct/sufficient thing to do, in order to have my patch landed? (I don't have any commit access, this is my 1st mozilla.org contribution.)
Attachment #8997255 - Attachment is obsolete: true
Attachment #8997255 - Flags: checked-in?
Attachment #8997630 - Flags: review?(rrelyea)
Attachment #8997630 - Flags: checked-in?
Comment on attachment 8997630 [details] [diff] [review]
Replace comment with pragma message; replace "key.h" with "keyhi.h"; replace "keyt.h" with "keythi.h"

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

I'm fine with this combined patch for both and fixing both issues with this bug.
I've made a comment about a separate issue that your cleanup exposed. I don't think you need to fix that separate issue (it's a longstanding, decades old NSS style standard violation) with this patch or in this bug. I'll write a separate bug for that issue. It can get worked on whenever.

bob

::: lib/pkcs12/p12t.h
@@ +6,4 @@
>  #define _P12T_H_
>  
>  #include "secoid.h"
> +#include "keyhi.h"

This should really be keyhit.h, since p12t.h should be types only. This is a preexisting problem and not the one you are trying to solve, so your patch is fine I'll write a separate bug for this cleanup.

::: lib/pkcs12/pkcs12t.h
@@ +8,4 @@
>  #include "seccomon.h"
>  #include "secoid.h"
>  #include "cert.h"
> +#include "keyhi.h"

same comment as p12t.h
Attachment #8997630 - Flags: review?(rrelyea) → review+
This can get landed when Bob's nits were addressed.
Assignee: nobody → ctruta
Flags: needinfo?(ctruta)
Ok, thank you, Franziskus. Just to be sure my understanding is correct: Bob needs that other issue to be resolved first, then this patch can be landed. But there's no further action required on my part, right?

Out of curiosity, what is that other issue?
Flags: needinfo?(ctruta) → needinfo?(franziskuskiefer)
Nope, you only need to address the two nits from comment 4, then your patch can get landed. Bob will file something for the other issue (moving the pragma). That is independent of this patch.
Flags: needinfo?(franziskuskiefer)
Ok, Franziskus, thanks for clarifying. I updated my patch, and I added you as a reviewer also.
Attachment #8997630 - Attachment is obsolete: true
Attachment #8997630 - Flags: checked-in?
Attachment #9001643 - Flags: review?(rrelyea)
Attachment #9001643 - Flags: review?(franziskuskiefer)
Attachment #9001643 - Flags: checked-in?
Oops... typo. It should have been "keythi.h", not "keyhit.h". Fixed.
Attachment #9001643 - Attachment is obsolete: true
Attachment #9001643 - Flags: review?(rrelyea)
Attachment #9001643 - Flags: review?(franziskuskiefer)
Attachment #9001643 - Flags: checked-in?
Attachment #9001678 - Flags: review?(rrelyea)
Attachment #9001678 - Flags: review?(franziskuskiefer)
Attachment #9001678 - Flags: checked-in?
Keywords: checkin-needed
Comment on attachment 9001678 [details] [diff] [review]
Replace comment with pragma message; replace "key.h" with "keyhi.h"; replace "keyt.h" with "keythi.h"

Nits addressed. Ready for check-in.
Attachment #9001678 - Flags: review?(rrelyea)
Attachment #9001678 - Flags: review?(franziskuskiefer)
Attachment #9001678 - Flags: review+
Attachment #9001678 - Flags: checked-in?
I'll take care of landing.
Status: UNCONFIRMED → NEW
Ever confirmed: true
https://hg.mozilla.org/projects/nss/rev/fe738aae0bcc
Status: NEW → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 3.40
Cool! Thanks, Kai :-)
You need to log in before you can comment on or make changes to this bug.