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

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Kai Engert (:KaiE:)]

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:)]

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:)]

Attachment: 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:)]

FYI,
  head --lines=-1 filename
will print the contents of "filename" except the trailing line.

Comment 4

[Mike Hommey [:glandium]]

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:)]

Attachment: Patch v2

Comment 6

[Kai Engert (:KaiE:)]

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:)]

Attachment: Patch v3

This appears to work fine.

Comment 8

[Mike Hommey [:glandium]]

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]]

(In reply to Mike Hommey [:glandium] from comment #8)
> if not lines[-1]:

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

Comment 10

[Kai Engert (:KaiE:)]

> > +  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:)]

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

Seems it is, mysterious python!

Comment 12

[Kai Engert (:KaiE:)]

Attachment: Patch v4

Comment 13

[Kai Engert (:KaiE:)]

(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]]

(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:)]

Attachment: patch v5

sorry, had missed that comment

thanks for your review

Comment 16

[Mike Hommey [:glandium]]

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:)]

Attachment: patch v6

thanks Mike! final fixes applied, and unnecessary test commands removed.

Comment 18

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc01b7e45d03

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bc01b7e45d03

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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:)]

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