Last Comment Bug 782784 - client.py update_nss and update_nspr should toggle the trailing whitespace line
: client.py update_nss and update_nspr should toggle the trailing whitespace line
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: 16 Branch
: All All
: -- normal (vote)
: mozilla17
Assigned To: Kai Engert (:kaie)
:
Mentors:
Depends on:
Blocks: 882101
  Show dependency treegraph
 
Reported: 2012-08-14 14:21 PDT by Kai Engert (:kaie)
Modified: 2013-06-12 04:46 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (3.66 KB, patch)
2012-08-14 14:35 PDT, Kai Engert (:kaie)
mh+mozilla: review-
Details | Diff | Splinter Review
Patch v2 (3.03 KB, patch)
2012-08-14 23:07 PDT, Kai Engert (:kaie)
kaie: review-
Details | Diff | Splinter Review
Patch v3 (3.05 KB, patch)
2012-08-14 23:34 PDT, Kai Engert (:kaie)
mh+mozilla: review-
Details | Diff | Splinter Review
Patch v4 (2.82 KB, patch)
2012-08-15 13:10 PDT, Kai Engert (:kaie)
no flags Details | Diff | Splinter Review
patch v5 (2.80 KB, patch)
2012-08-15 13:57 PDT, Kai Engert (:kaie)
mh+mozilla: review+
Details | Diff | Splinter Review
patch v6 (2.60 KB, patch)
2012-08-15 23:26 PDT, Kai Engert (:kaie)
kaie: review+
Details | Diff | Splinter Review

Description Kai Engert (:kaie) 2012-08-14 14:21:04 PDT
The NSPR and NSS dependency system is unreliable.

Whenever we update Mozilla to use a new snapshot of NSPR or NSS,
we must remember to add or remove a whitespace line to the respective trick header file,
in order to force a full rebuild.

(as documented at https://developer.mozilla.org/en/Updating_NSPR_or_NSS_in_mozilla-central )

I've repeatedly forgotten to do that, therefore I want it automated.

The proposal is to change the udpate_nss and update_nspr commands in client.py,
and automatically add a blank line (or delete a blank line) at the end of the file.

For NSPR the file is mozilla/nsprpub/config/prdepend.h

For NSS the file is mozilla/security/coreconf/coreconf.dep
Comment 1 Kai Engert (:kaie) 2012-08-14 14:23:04 PDT
I wrote this code in a bash script:

TAIL_EMPTY_COUNT=`tail -2 security/coreconf/coreconf.dep  |grep -c "^$"`

if [ $TAIL_EMPTY_COUNT == 2 ]; then
  # remove
  head --lines=-1 security/coreconf/coreconf.dep > security/coreconf/coreconf.dep.tmp
  mv -f security/coreconf/coreconf.dep.tmp security/coreconf/coreconf.dep
else
  # append
  echo "" >> security/coreconf/coreconf.dep
fi


But client.py is python, and I found a way to translate the above bash into python:


import subprocess

depname = "security/coreconf/coreconf.dep"
tmpname = depname + ".tmp"

p1 = subprocess.Popen(["tail", "-2", depname], stdout=subprocess.PIPE)
p2 = subprocess.Popen(["grep", "-c", "^$"], stdin=p1.stdout, stdout=subprocess.PIPE)
p1.stdout.close()  # Allow p1 to receive a SIGPIPE if p2 exits.
output = p2.communicate()[0].strip()

if output == "2":
  print "deleting"
  with open(tmpname, "w") as tmpfile:
    subprocess.call(["head", "--lines=-1", depname], stdout=tmpfile)
    tmpfile.close()
  subprocess.call(["mv", "-f", tmpname, depname])
else:
  print "appending"
  with open(depname, "a") as appendfile:
    appendfile.write("\n")
    appendfile.close()


This works for me, I'll attach a patch that integrates this logic into client.py
Comment 2 Kai Engert (:kaie) 2012-08-14 14:35:43 PDT
Created attachment 651890 [details] [diff] [review]
Patch v1

Can you please review for mistakes?
Thanks

I've included toggle_nspr and toggle_nss commands, allowing you to easily test on your own.
We can remove these commands if you don't like them.
Comment 3 Kai Engert (:kaie) 2012-08-14 14:37:08 PDT
FYI,
  head --lines=-1 filename
will print the contents of "filename" except the trailing line.
Comment 4 Mike Hommey [:glandium] 2012-08-14 15:00:50 PDT
Comment on attachment 651890 [details] [diff] [review]
Patch v1

Please use python APIs instead of calling external processes (with options not supported on all platforms, on top of that (head --lines is a GNUism, it's not supported on OSX, at the very least))
Comment 5 Kai Engert (:kaie) 2012-08-14 23:07:22 PDT
Created attachment 652008 [details] [diff] [review]
Patch v2
Comment 6 Kai Engert (:kaie) 2012-08-14 23:29:37 PDT
Comment on attachment 652008 [details] [diff] [review]
Patch v2

sigh. while trying to understand the surprising behaviour of my script, I discovered the output of "range" is one less than I had expected.
Comment 7 Kai Engert (:kaie) 2012-08-14 23:34:34 PDT
Created attachment 652014 [details] [diff] [review]
Patch v3

This appears to work fine.
Comment 8 Mike Hommey [:glandium] 2012-08-15 00:08:20 PDT
Comment on attachment 652014 [details] [diff] [review]
Patch v3

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

::: client.py
@@ +91,5 @@
> +  Otherwise we'll add a blank line."""
> +  f = open(depname, "r")
> +  lines = []
> +  for line in f:
> +      lines.append(line.rstrip("\n"))

lines = open(depname, "r").readlines()

@@ +98,5 @@
> +  if numlines < 1:
> +      print "unexpected short file"
> +      return
> +
> +  if (lines[numlines-1].strip() == ''):

if not lines[-1]:

@@ +102,5 @@
> +  if (lines[numlines-1].strip() == ''):
> +      # trailing line is blank, removing it
> +      with open(depname, "w") as outfile:
> +          for i in range(0, numlines-1):
> +              outfile.write("%s\n" % lines[i]);

outfile.writelines(lines[:-1])

@@ +103,5 @@
> +      # trailing line is blank, removing it
> +      with open(depname, "w") as outfile:
> +          for i in range(0, numlines-1):
> +              outfile.write("%s\n" % lines[i]);
> +          outfile.close()

when using with, you don't close. And since that only leaves one line under with, you might as well do open(depname, "w").writelines(lines[:-1])

@@ +108,5 @@
> +  else:
> +      # adding blank line
> +      with open(depname, "a") as appendfile:
> +          appendfile.write("\n")
> +          appendfile.close()

likewise
Comment 9 Mike Hommey [:glandium] 2012-08-15 00:09:48 PDT
(In reply to Mike Hommey [:glandium] from comment #8)
> if not lines[-1]:

lines[-1].strip(), even.
Comment 10 Kai Engert (:kaie) 2012-08-15 13:06:20 PDT
> > +  if (lines[numlines-1].strip() == ''):
> 
> if not lines[-1]:

Is is really equivalent with the statement involving strip()?
What if the line contains whitespace only?
Comment 11 Kai Engert (:kaie) 2012-08-15 13:09:11 PDT
> Is is really equivalent with the statement involving strip()?

Seems it is, mysterious python!
Comment 12 Kai Engert (:kaie) 2012-08-15 13:10:25 PDT
Created attachment 652192 [details] [diff] [review]
Patch v4
Comment 13 Kai Engert (:kaie) 2012-08-15 13:14:33 PDT
(In reply to Kai Engert (:kaie) from comment #11)
> > Is is really equivalent with the statement involving strip()?
> 
> Seems it is, mysterious python!

No, I had not made that change. Now I tested your proposal, and it's not equivalent.

I suggest to use patch v4 (which doesn't use your proposal),
because my approach will also remove a whitespace-only lines.
Comment 14 Mike Hommey [:glandium] 2012-08-15 13:41:34 PDT
(In reply to Kai Engert (:kaie) from comment #13)
> (In reply to Kai Engert (:kaie) from comment #11)
> > > Is is really equivalent with the statement involving strip()?
> > 
> > Seems it is, mysterious python!
> 
> No, I had not made that change. Now I tested your proposal, and it's not
> equivalent.
> 
> I suggest to use patch v4 (which doesn't use your proposal),
> because my approach will also remove a whitespace-only lines.

See comment 9.
Comment 15 Kai Engert (:kaie) 2012-08-15 13:57:00 PDT
Created attachment 652218 [details] [diff] [review]
patch v5

sorry, had missed that comment

thanks for your review
Comment 16 Mike Hommey [:glandium] 2012-08-15 23:17:56 PDT
Comment on attachment 652218 [details] [diff] [review]
patch v5

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

::: client.py
@@ +86,5 @@
>                           cwd=os.path.join(topsrcdir, parent))
>          print "CVS export end: " + datetime.datetime.utcnow().strftime("%Y-%m-%d %H:%M:%S UTC")
>  
> +def toggle_trailing_blank_line(depname):
> +  """If the trailing line is re empty, then we'll delete it.

"re empty" ?

@@ +90,5 @@
> +  """If the trailing line is re empty, then we'll delete it.
> +  Otherwise we'll add a blank line."""
> +  lines = open(depname, "r").readlines()
> +  numlines = len(lines)
> +  if numlines < 1:

if not lines

@@ +91,5 @@
> +  Otherwise we'll add a blank line."""
> +  lines = open(depname, "r").readlines()
> +  numlines = len(lines)
> +  if numlines < 1:
> +      print "unexpected short file"

print >>sys.stderr, "...
Comment 17 Kai Engert (:kaie) 2012-08-15 23:26:21 PDT
Created attachment 652333 [details] [diff] [review]
patch v6

thanks Mike! final fixes applied, and unnecessary test commands removed.
Comment 19 Ed Morley [:emorley] 2012-08-16 06:16:44 PDT
https://hg.mozilla.org/mozilla-central/rev/bc01b7e45d03
Comment 20 :Ms2ger (⌚ UTC+1/+2) 2012-08-16 10:21:57 PDT
Comment on attachment 652333 [details] [diff] [review]
patch v6

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

::: client.py
@@ +91,5 @@
> +  Otherwise we'll add a blank line."""
> +  lines = open(depname, "r").readlines()
> +  if not lines:
> +      print >>sys.stderr, "unexpected short file"
> +      return

Weird indentation
Comment 21 Kai Engert (:kaie) 2012-10-18 12:42:26 PDT
m2sger: instead of saying just "weird", it would be good if you rather clarified what you find weird about it, and what you propose instead

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