Issue 2956 - Ensure the correct version of jsshell is present

Issue 2956 - Ensure the correct version of jsshell is present

[kzar, 2015-11-18 14:58:06]

Patch Set 1

[sergei, 2015-11-18 15:30:07]

https://codereview.adblockplus.org/29330389/diff/29330390/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29330389/diff/29330390/utils.py#newcode35
utils.py:35: if version == "JavaScript-C31.5.0esrpre":
Nit: I don't know the convention for buildtools but I would read this version
string from some text file. So, the updater of jsshell need to edit this file
instead of python script. Seems also better for the history log.

https://codereview.adblockplus.org/29330389/diff/29330390/utils.py#newcode35
utils.py:35: if version == "JavaScript-C31.5.0esrpre":
Nit: In addition it seems better to show some warning explaining what happened.
Otherwise, I'm afraid, the updater will have to spend some time investigating
who has reverted just updated version?...

I would not like to overcomplicate it, however I would like to mention the
following dirty solution. The updater should append the line with a new version.
So
if version list is not empty:
  if version is not in version list:
    # instead of downgrading show the warning
    warning("Don't forget to update jsshell.version")
  else if version is not last entry in version list:
    trace("Updating jsshell")
    rmtree...

[kzar, 2015-11-18 15:40:50]

https://codereview.adblockplus.org/29330389/diff/29330390/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29330389/diff/29330390/utils.py#newcode35
utils.py:35: if version == "JavaScript-C31.5.0esrpre":
On 2015/11/18 15:30:06, sergei wrote:
> Nit: I don't know the convention for buildtools but I would read this version
> string from some text file. So, the updater of jsshell need to edit this file
> instead of python script. Seems also better for the history log.

Well I would have separated the version string from the code, but as the
downloaded version is also determined by a hard-coded string in this script I
didn't see the point.

As for showing a warning instead of upgrading jsshell, I disagree. I think that
the behaviour I implemented is more consistent with how it currently works. (Ie
the correct version of jsshell is grabbed magically in the background.)

[sergei, 2015-11-18 15:50:18]

https://codereview.adblockplus.org/29330389/diff/29330390/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29330389/diff/29330390/utils.py#newcode35
utils.py:35: if version == "JavaScript-C31.5.0esrpre":
On 2015/11/18 15:40:49, kzar wrote:
> On 2015/11/18 15:30:06, sergei wrote:
> > Nit: I don't know the convention for buildtools but I would read this
version
> > string from some text file. So, the updater of jsshell need to edit this
file
> > instead of python script. Seems also better for the history log.
> 
> Well I would have separated the version string from the code, but as the
> downloaded version is also determined by a hard-coded string in this script I
> didn't see the point.
I have missed it, in this case it's totally fine for me and, I guess, we can
omit the downgrading warning.

> As for showing a warning instead of upgrading jsshell, I disagree. I think
that
> the behaviour I implemented is more consistent with how it currently works.
(Ie
> the correct version of jsshell is grabbed magically in the background.)
I meant the warning about downgrading, upgrading can be quite.

[Sebastian Noack, 2015-11-18 19:53:37]

https://codereview.adblockplus.org/29330389/diff/29330390/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29330389/diff/29330390/utils.py#newcode30
utils.py:30: version = [s for s in subprocess.check_output([path,
"--help"]).split("\n")
This can be simplified:

  m = re.search(r"^Version:.*(\d+)", subprocess.check_output([path, "--help"]),
re.MULTILINE)
  if m and m.group(1) == "31":
    ..

Also consider moving the version as well as the URL into a global at the top of
the file to make it easier to find the stuff that need to be changed when
updating.

[kzar, 2015-11-19 10:56:42]

Patch Set 2 : Use regexp to match version number and tidy things up

https://codereview.adblockplus.org/29330389/diff/29330390/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29330389/diff/29330390/utils.py#newcode30
utils.py:30: version = [s for s in subprocess.check_output([path,
"--help"]).split("\n")
On 2015/11/18 19:53:37, Sebastian Noack wrote:
> This can be simplified:
> 
>   m = re.search(r"^Version:.*(\d+)", subprocess.check_output([path,
"--help"]),
> re.MULTILINE)
>   if m and m.group(1) == "31":
>     ..
> 
> Also consider moving the version as well as the URL into a global at the top
of
> the file to make it easier to find the stuff that need to be changed when
> updating.

Done.

[Sebastian Noack, 2015-11-19 20:21:43]

https://codereview.adblockplus.org/29330389/diff/29330416/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29330389/diff/29330416/utils.py#newcode39
utils.py:39: shutil.rmtree(shell_dir)
I'm not sure whether this is a good idea. Imagine you work offline and switch to
a revision that requires a different version of jsshell, then the previous
version is deleted, the new one fails to download, and you cannot go back
either.

[kzar, 2015-11-20 09:02:00]

https://codereview.adblockplus.org/29330389/diff/29330416/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29330389/diff/29330416/utils.py#newcode39
utils.py:39: shutil.rmtree(shell_dir)
On 2015/11/19 20:21:43, Sebastian Noack wrote:
> I'm not sure whether this is a good idea. Imagine you work offline and switch
to
> a revision that requires a different version of jsshell, then the previous
> version is deleted, the new one fails to download, and you cannot go back
> either. 

That's true, but the current behaviour is to somewhat magically (and silently)
download jsshell in the background as well. When I had trouble connecting to the
Mozilla FTP server the other day my build was silently hanging while it tried to
download and it took me a while to pin down.

I don't mind changing that, but we need to agree on the desired behaviour. Any
ideas? (Also that should arguably be a separate issue.)

[kzar, 2015-11-23 12:40:49]

Patch Set 3 : Base directory name on jsshell version, avoid checking which
version the binary outputs

[Sebastian Noack, 2015-11-23 12:53:59]

https://codereview.adblockplus.org/29330389/diff/29330679/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29330389/diff/29330679/utils.py#newcode29
utils.py:29: shell_dir = os.path.join(baseDir, "jsshell-%s" % JSSHELL_VERSION)
With the new approach, hard-coding the version is redundant. On the other hand
calling "js --help" to automatically get the version would add some more
complexity. But how about using the "2015-02-25-00-22-19-mozilla-esr31" part
from the URL as directory name?

https://codereview.adblockplus.org/29330389/diff/29330679/utils.py#newcode50
utils.py:50: data = StringIO(urllib.urlopen(JSSHELL_URL % build).read())
While changing this code anyway, mind closing the file-like object returned by
urlopen()?

[kzar, 2015-11-23 13:20:22]

Patch Set 4 : Make sure to close the urllib response object

https://codereview.adblockplus.org/29330389/diff/29330679/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29330389/diff/29330679/utils.py#newcode29
utils.py:29: shell_dir = os.path.join(baseDir, "jsshell-%s" % JSSHELL_VERSION)
On 2015/11/23 12:53:58, Sebastian Noack wrote:
> With the new approach, hard-coding the version is redundant. On the other hand
> calling "js --help" to automatically get the version would add some more
> complexity. But how about using the "2015-02-25-00-22-19-mozilla-esr31" part
> from the URL as directory name?

Hmm I see what you mean, but on the other hand having the directory called
something like jsshell-2015-02-25-00-22-19-mozilla-esr31/ seems pretty ugly
compared to jsshell-31.5.0/. Mind if I leave this as it is?

https://codereview.adblockplus.org/29330389/diff/29330679/utils.py#newcode50
utils.py:50: data = StringIO(urllib.urlopen(JSSHELL_URL % build).read())
On 2015/11/23 12:53:58, Sebastian Noack wrote:
> While changing this code anyway, mind closing the file-like object returned by
> urlopen()?

Done.

[Sebastian Noack, 2015-11-23 13:27:48]

https://codereview.adblockplus.org/29330389/diff/29330679/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29330389/diff/29330679/utils.py#newcode29
utils.py:29: shell_dir = os.path.join(baseDir, "jsshell-%s" % JSSHELL_VERSION)
On 2015/11/23 13:20:21, kzar wrote:
> On 2015/11/23 12:53:58, Sebastian Noack wrote:
> > With the new approach, hard-coding the version is redundant. On the other
hand
> > calling "js --help" to automatically get the version would add some more
> > complexity. But how about using the "2015-02-25-00-22-19-mozilla-esr31" part
> > from the URL as directory name?
> 
> Hmm I see what you mean, but on the other hand having the directory called
> something like jsshell-2015-02-25-00-22-19-mozilla-esr31/ seems pretty ugly
> compared to jsshell-31.5.0/. Mind if I leave this as it is?

How about following?

JSSHELL_DIR = "mozilla-esr31"
JSSHELL_URL = "..." % JSSHELL_DIR

[Sebastian Noack, 2015-11-23 13:31:00]

https://codereview.adblockplus.org/29330389/diff/29330697/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29330389/diff/29330697/utils.py#newcode52
utils.py:52: ZipFile(StringIO(urllib.urlopen(JSSHELL_URL % build).read())) as
zip:
Ehmm... Guess what's wrong with that? ;)

[kzar, 2015-11-23 13:38:12]

https://codereview.adblockplus.org/29330389/diff/29330679/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29330389/diff/29330679/utils.py#newcode29
utils.py:29: shell_dir = os.path.join(baseDir, "jsshell-%s" % JSSHELL_VERSION)
On 2015/11/23 13:27:48, Sebastian Noack wrote:
> On 2015/11/23 13:20:21, kzar wrote:
> > On 2015/11/23 12:53:58, Sebastian Noack wrote:
> > > With the new approach, hard-coding the version is redundant. On the other
> hand
> > > calling "js --help" to automatically get the version would add some more
> > > complexity. But how about using the "2015-02-25-00-22-19-mozilla-esr31"
part
> > > from the URL as directory name?
> > 
> > Hmm I see what you mean, but on the other hand having the directory called
> > something like jsshell-2015-02-25-00-22-19-mozilla-esr31/ seems pretty ugly
> > compared to jsshell-31.5.0/. Mind if I leave this as it is?
> 
> How about following?
> 
> JSSHELL_DIR = "mozilla-esr31"
> JSSHELL_URL = "..." % JSSHELL_DIR

That's better but still kind of sucks, the directory would be mozilla-esr31/
instead of jsshell-31.5.0/ which IMO is less descriptive. I also don't like how
the directory name would then be tied to the URL. If we wanted to upgrade to
jsshell 31.6.0 for example we would be stuck using the same directory name.

[kzar, 2015-11-23 13:44:11]

Patch Set 5 : Don't download jsshell twice!

https://codereview.adblockplus.org/29330389/diff/29330697/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29330389/diff/29330697/utils.py#newcode52
utils.py:52: ZipFile(StringIO(urllib.urlopen(JSSHELL_URL % build).read())) as
zip:
On 2015/11/23 13:31:00, Sebastian Noack wrote:
> Ehmm... Guess what's wrong with that? ;)

Argh, whoops!

[Sebastian Noack, 2015-11-23 13:51:16]

https://codereview.adblockplus.org/29330389/diff/29330679/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29330389/diff/29330679/utils.py#newcode29
utils.py:29: shell_dir = os.path.join(baseDir, "jsshell-%s" % JSSHELL_VERSION)
On 2015/11/23 13:38:12, kzar wrote:
> On 2015/11/23 13:27:48, Sebastian Noack wrote:
> > On 2015/11/23 13:20:21, kzar wrote:
> > > On 2015/11/23 12:53:58, Sebastian Noack wrote:
> > > > With the new approach, hard-coding the version is redundant. On the
other
> > hand
> > > > calling "js --help" to automatically get the version would add some more
> > > > complexity. But how about using the "2015-02-25-00-22-19-mozilla-esr31"
> part
> > > > from the URL as directory name?
> > > 
> > > Hmm I see what you mean, but on the other hand having the directory called
> > > something like jsshell-2015-02-25-00-22-19-mozilla-esr31/ seems pretty
ugly
> > > compared to jsshell-31.5.0/. Mind if I leave this as it is?
> > 
> > How about following?
> > 
> > JSSHELL_DIR = "mozilla-esr31"
> > JSSHELL_URL = "..." % JSSHELL_DIR
> 
> That's better but still kind of sucks, the directory would be mozilla-esr31/
> instead of jsshell-31.5.0/ which IMO is less descriptive. I also don't like
how
> the directory name would then be tied to the URL. If we wanted to upgrade to
> jsshell 31.6.0 for example we would be stuck using the same directory name.

Then include the timestamp if that is your concern as I initially suggested.
However independently/redundantly configuring the version isn't justified just
for a leaner dirnames. Also note that the current dirnames is just "mozilla" and
improving the semantics of it isn't matter of this patch anyway.

https://codereview.adblockplus.org/29330389/diff/29330679/utils.py#newcode29
utils.py:29: shell_dir = os.path.join(baseDir, "jsshell-%s" % JSSHELL_VERSION)
On 2015/11/23 13:38:12, kzar wrote:
> On 2015/11/23 13:27:48, Sebastian Noack wrote:
> > On 2015/11/23 13:20:21, kzar wrote:
> > > On 2015/11/23 12:53:58, Sebastian Noack wrote:
> > > > With the new approach, hard-coding the version is redundant. On the
other
> > hand
> > > > calling "js --help" to automatically get the version would add some more
> > > > complexity. But how about using the "2015-02-25-00-22-19-mozilla-esr31"
> part
> > > > from the URL as directory name?
> > > 
> > > Hmm I see what you mean, but on the other hand having the directory called
> > > something like jsshell-2015-02-25-00-22-19-mozilla-esr31/ seems pretty
ugly
> > > compared to jsshell-31.5.0/. Mind if I leave this as it is?
> > 
> > How about following?
> > 
> > JSSHELL_DIR = "mozilla-esr31"
> > JSSHELL_URL = "..." % JSSHELL_DIR
> 
> That's better but still kind of sucks, the directory would be mozilla-esr31/
> instead of jsshell-31.5.0/ which IMO is less descriptive. I also don't like
how
> the directory name would then be tied to the URL. If we wanted to upgrade to
> jsshell 31.6.0 for example we would be stuck using the same directory name.

Then include the timestamp if that is your concern as I initially suggested.
However independently/redundantly configuring the version isn't justified just
for a leaner dirname. Also note that the current dirnames is just "mozilla" and
improving the semantics of it isn't matter of this change anyway.

[kzar, 2015-11-23 14:01:31]

Patch Set 6 : Remove JSSHELL_VERSION variable

https://codereview.adblockplus.org/29330389/diff/29330679/utils.py
File utils.py (right):

https://codereview.adblockplus.org/29330389/diff/29330679/utils.py#newcode29
utils.py:29: shell_dir = os.path.join(baseDir, "jsshell-%s" % JSSHELL_VERSION)
On 2015/11/23 13:51:16, Sebastian Noack wrote:
> On 2015/11/23 13:38:12, kzar wrote:
> > On 2015/11/23 13:27:48, Sebastian Noack wrote:
> > > On 2015/11/23 13:20:21, kzar wrote:
> > > > On 2015/11/23 12:53:58, Sebastian Noack wrote:
> > > > > With the new approach, hard-coding the version is redundant. On the
> other
> > > hand
> > > > > calling "js --help" to automatically get the version would add some
more
> > > > > complexity. But how about using the
"2015-02-25-00-22-19-mozilla-esr31"
> > part
> > > > > from the URL as directory name?
> > > > 
> > > > Hmm I see what you mean, but on the other hand having the directory
called
> > > > something like jsshell-2015-02-25-00-22-19-mozilla-esr31/ seems pretty
> ugly
> > > > compared to jsshell-31.5.0/. Mind if I leave this as it is?
> > > 
> > > How about following?
> > > 
> > > JSSHELL_DIR = "mozilla-esr31"
> > > JSSHELL_URL = "..." % JSSHELL_DIR
> > 
> > That's better but still kind of sucks, the directory would be mozilla-esr31/
> > instead of jsshell-31.5.0/ which IMO is less descriptive. I also don't like
> how
> > the directory name would then be tied to the URL. If we wanted to upgrade to
> > jsshell 31.6.0 for example we would be stuck using the same directory name.
> 
> Then include the timestamp if that is your concern as I initially suggested.
> However independently/redundantly configuring the version isn't justified just
> for a leaner dirnames. Also note that the current dirnames is just "mozilla"
and
> improving the semantics of it isn't matter of this patch anyway.

Fine, Done.

[Sebastian Noack, 2015-11-23 14:19:56]

LGTM

[Wladimir Palant, 2015-11-24 09:25:20]

Actually, if we are using different directory names already it would make sense
to have the platform in there as well (Linux and OS X versions of JS shell
cannot be distinguished otherwise). Well, maybe some other time.

[Sebastian Noack, 2015-11-24 09:35:54]

On 2015/11/24 09:25:20, Wladimir Palant wrote:
> Actually, if we are using different directory names already it would make
sense
> to have the platform in there as well (Linux and OS X versions of JS shell
> cannot be distinguished otherwise). Well, maybe some other time.

I agree. That way things will still work when the repo is copied across
machines. Since this change already landed, feel free to submit another patch.

[kzar, 2015-11-24 10:33:24]

There you go https://codereview.adblockplus.org/29330730/