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)
:
: Gregory Szorc [:gps] (away until 2017-03-20)
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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Kai Engert (:kaie) 2012-08-14 23:07:22 PDT
Created attachment 652008 [details] [diff] [review]
Patch v2
Comment 6 User image 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 User image 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 User image 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 User image 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 User image 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 User image 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 User image Kai Engert (:kaie) 2012-08-15 13:10:25 PDT
Created attachment 652192 [details] [diff] [review]
Patch v4
Comment 13 User image 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 User image 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 User image 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 User image 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 User image 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 User image Ed Morley [:emorley] 2012-08-16 06:16:44 PDT
https://hg.mozilla.org/mozilla-central/rev/bc01b7e45d03
Comment 20 User image :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 User image 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.