Last Comment Bug 699905 - client.py - update_nssckbi - security/nss/TAG-INFO-CKBI
: client.py - update_nssckbi - security/nss/TAG-INFO-CKBI
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Security: PSM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Kai Engert (:kaie)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-04 13:26 PDT by Kai Engert (:kaie)
Modified: 2012-02-20 10:23 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (1.67 KB, patch)
2011-11-04 13:26 PDT, Kai Engert (:kaie)
brian: review+
Details | Diff | Splinter Review
Patch v2 (1.67 KB, patch)
2012-02-17 08:21 PST, Kai Engert (:kaie)
kaie: review+
wtc: superreview+
Details | Diff | Splinter Review
Patch v3 (1.67 KB, patch)
2012-02-17 17:38 PST, Kai Engert (:kaie)
kaie: review+
kaie: superreview+
Details | Diff | Splinter Review

Description Kai Engert (:kaie) 2011-11-04 13:26:20 PDT
Created attachment 572059 [details] [diff] [review]
Patch v1

In the Mozilla application code, we sometimes apply a newer NSS roots module on top of the most recent NSS release.

When we do so, this information is difficult to notice in the code.
In order to improve visibility, I propose the following change:

- in addition to our existing security/nss/TAG-INFO
  introduce an additional file security/nss/TAG-CKBI-INFO

- file TAG-CKBI-INFO should contain the tag used for directory
  mozilla/security/nss/lib/ckfw/builtins

- whenever we use the old command update_nss
  (which will update all of NSS including the roots module)
  then let's write this tag into both files

- whenever we update only the roots module by using command update_nssckbi
  then let's this the tag to the new file TAG-CKBI-INFO


This new command has another benefit:

In the future, we don't need to deal with individual patches on the various Mozilla release branches. We can tell people to simply run this command to import the newer roots module.

Patch attached
Comment 1 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-02-15 14:08:19 PST
Comment on attachment 572059 [details] [diff] [review]
Patch v1

LGTM. Since I know very little about how the root database module works, Bob should verify that the directories are the correct ones.
Comment 2 Wan-Teh Chang 2012-02-15 14:27:00 PST
Comment on attachment 572059 [details] [diff] [review]
Patch v1

>+    do_cvs_export(NSSCKBI_DIRS, tag, options.cvsroot, options.cvs)
>+    print >>file("security/nss/TAG-CKBI-INFO", "w"), tag

Kai, I recommend adding security/nss/lib/ckfw/builtins/TAG-INFO instead.
Comment 3 Wan-Teh Chang 2012-02-15 14:28:53 PST
To elaborate on my recommendation: by using the same file name,
we can locate all the tag info files using a simple query like
http://mxr.mozilla.org/mozilla-central/find?text=&string=TAG-INFO
Comment 4 Kai Engert (:kaie) 2012-02-15 14:31:13 PST
(In reply to Wan-Teh Chang from comment #2)
> 
> Kai, I recommend adding security/nss/lib/ckfw/builtins/TAG-INFO instead.

Well, here is my motivation for the patch as is:

Only the NSS developers know about this "weird" separate versioning of that builtins directory.
If we hide that file inside that subdirectory, nobody will find it, except us experts.

By having both files next to each other in the security/nss directories, everyone will immediately notice there are two separate versions involved, even without prior education.
Comment 5 Kai Engert (:kaie) 2012-02-15 14:33:00 PST
(In reply to Wan-Teh Chang from comment #3)
> To elaborate on my recommendation: by using the same file name,
> we can locate all the tag info files using a simple query like
> http://mxr.mozilla.org/mozilla-central/find?text=&string=TAG-INFO

Sure, but are "we" really the target audience, or is the target audience tghe people who want to quickly learn what NSPR/NSS versions are used by a given Firefox source snapshot? In my opinion it's the latter.

The world is aware that we have separate NSPR and NSS libaries, but they probably not aware that we have this "separate versioned builtin roots module" thing.
Comment 6 Robert Relyea 2012-02-15 16:47:35 PST
I think I need a little background here:

Are these file files that are in our CVS tree, or are they the files added to our tar balls, or are they files that are imported into Hg?

From wtc's comments, I suspect it's that last.

Since the file is added by script, it would be quite simple to get both worlds either by:
1) Making 2 copies. or
2) Making 1 copy at wtc's location and adding a link from kai's name (assume Hg handles symbolic links).
3) Making 1 copy at wtc's location, and a second 'pointer file' which tells the user where the second one is.

bob
Comment 7 Wan-Teh Chang 2012-02-15 19:03:40 PST
Kai: I believe the file name CKBI-TAG-INFO would also allow
my MXR query in comment 3 to find it.  We should only add
one copy of this file.  We can add it to the location you
proposed.
Comment 8 Kai Engert (:kaie) 2012-02-16 09:03:51 PST
Bob, I will explain it to you in today's NSS conf call.
Comment 9 Kai Engert (:kaie) 2012-02-16 09:06:15 PST
(In reply to Wan-Teh Chang from comment #7)
> Kai: I believe the file name CKBI-TAG-INFO would also allow
> my MXR query in comment 3 to find it.

Wan-Teh, I really like your creative proposal, thank you very much for this idea!
I will attach a new patch.

I think both CKBI-TAG-INFO and TAG-INFO-CKBI will work.
Do you have a preference between these two alternatives?


> We can add it to the location you proposed.

Thank you!
Comment 10 Kai Engert (:kaie) 2012-02-17 08:21:02 PST
Created attachment 598244 [details] [diff] [review]
Patch v2

Carrying forward r=bsmith

I've implemented Wan-Teh's change request.
Comment 11 Wan-Teh Chang 2012-02-17 14:46:07 PST
Comment on attachment 598244 [details] [diff] [review]
Patch v2

r=wtc.  Note: TAG-INFO-CKBI would also work, and would allow the
two files to show up together in "ls" output.  You can pick the
one you prefer.
Comment 12 Wan-Teh Chang 2012-02-17 14:48:49 PST
I just noticed that you also suggested TAG-INFO-CKBI.
My preference is TAG-INFO-CKBI.
Comment 13 Kai Engert (:kaie) 2012-02-17 14:54:01 PST
Ok. Will use TAG-INFO-CKBI
Comment 14 Kai Engert (:kaie) 2012-02-17 17:38:27 PST
Created attachment 598457 [details] [diff] [review]
Patch v3

Updated patch to our preferred filename, carrying forward reviews.
Comment 15 Ed Morley [:emorley] 2012-02-18 16:35:03 PST
Commit message from IRC:
"Bug 699905 - Add update_nssckbi to client.py; r=bsmith r=wtc"

Will push this tomorrow if it hasn't otherwise been done by then :-)
Comment 17 Ed Morley [:emorley] 2012-02-20 10:23:17 PST
https://hg.mozilla.org/mozilla-central/rev/d138ca47dbe2

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