client.py update_nss and update_nspr should toggle the trailing whitespace line

RESOLVED FIXED in mozilla17

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: kaie, Assigned: kaie)

Tracking

16 Branch
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

5 years ago
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
(Assignee)

Comment 1

5 years ago
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
(Assignee)

Comment 2

5 years ago
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.
Assignee: nobody → kaie
Attachment #651890 - Flags: review?(wtc)
(Assignee)

Comment 3

5 years ago
FYI,
  head --lines=-1 filename
will print the contents of "filename" except the trailing line.
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))
Attachment #651890 - Flags: review?(wtc) → review-
(Assignee)

Comment 5

5 years ago
Created attachment 652008 [details] [diff] [review]
Patch v2
Attachment #651890 - Attachment is obsolete: true
Attachment #652008 - Flags: review?(mh+mozilla)
(Assignee)

Comment 6

5 years ago
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.
Attachment #652008 - Flags: review?(mh+mozilla) → review-
(Assignee)

Comment 7

5 years ago
Created attachment 652014 [details] [diff] [review]
Patch v3

This appears to work fine.
Attachment #652008 - Attachment is obsolete: true
Attachment #652014 - Flags: review?(mh+mozilla)
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
Attachment #652014 - Flags: review?(mh+mozilla) → review-
(In reply to Mike Hommey [:glandium] from comment #8)
> if not lines[-1]:

lines[-1].strip(), even.
(Assignee)

Comment 10

5 years ago
> > +  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?
(Assignee)

Comment 11

5 years ago
> Is is really equivalent with the statement involving strip()?

Seems it is, mysterious python!
(Assignee)

Comment 12

5 years ago
Created attachment 652192 [details] [diff] [review]
Patch v4
Attachment #652192 - Flags: review?(mh+mozilla)
(Assignee)

Updated

5 years ago
Attachment #652014 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
(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.
(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.
(Assignee)

Comment 15

5 years ago
Created attachment 652218 [details] [diff] [review]
patch v5

sorry, had missed that comment

thanks for your review
Attachment #652192 - Attachment is obsolete: true
Attachment #652192 - Flags: review?(mh+mozilla)
Attachment #652218 - Flags: review?(mh+mozilla)
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, "...
Attachment #652218 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 17

5 years ago
Created attachment 652333 [details] [diff] [review]
patch v6

thanks Mike! final fixes applied, and unnecessary test commands removed.
Attachment #652218 - Attachment is obsolete: true
Attachment #652333 - Flags: review+
(Assignee)

Comment 18

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc01b7e45d03
https://hg.mozilla.org/mozilla-central/rev/bc01b7e45d03
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
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
(Assignee)

Comment 21

5 years ago
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
(Assignee)

Updated

4 years ago
Blocks: 882101
You need to log in before you can comment on or make changes to this bug.