Issue 6523 - Update softlink for downloaded builds

Issue 6523 - Update softlink for downloaded builds

[tlucas, 2018-03-26 09:37:18]

Patch Set 1

 * Insert missing logic for updating the softlink

[Sebastian Noack, 2018-03-26 16:39:25]

LGTM

[Vasily Kuznetsov, 2018-03-26 19:12:12]

Hi Tristan,

Looks good but the way you store the path in `self.path` (and then read it from
their later) instead of returning it explicitly makes me a bit uneasy. This
creates a hidden dependency between the code in `download_from_mozilla_addons()`
and the code in `download()` and can potentially break since not all paths
through `download_from_mozilla_addons` set `self.path`.

What do you think about returning the path instead of passing it through
`self.path`?

Cheers,
Vasily

https://codereview.adblockplus.org/29733599/diff/29733600/sitescripts/extensi...
File sitescripts/extensions/bin/createNightlies.py (right):

https://codereview.adblockplus.org/29733599/diff/29733600/sitescripts/extensi...
sitescripts/extensions/bin/createNightlies.py:858:
self.symlink_or_copy(self.path, linkPath)
What if we don't have `self.path` because in `download_from_mozilla_addons` we
followed the `else` branch of the `if` statement at line 549?

[tlucas, 2018-03-27 07:54:59]

On 2018/03/26 19:12:12, Vasily Kuznetsov wrote:
> Hi Tristan,
> 
> Looks good but the way you store the path in `self.path` (and then read it
from
> their later) instead of returning it explicitly makes me a bit uneasy. This
> creates a hidden dependency between the code in
`download_from_mozilla_addons()`
> and the code in `download()` and can potentially break since not all paths
> through `download_from_mozilla_addons` set `self.path`.
> 
> What do you think about returning the path instead of passing it through
> `self.path`?
> 
> Cheers,
> Vasily
> 
>
https://codereview.adblockplus.org/29733599/diff/29733600/sitescripts/extensi...
> File sitescripts/extensions/bin/createNightlies.py (right):
> 
>
https://codereview.adblockplus.org/29733599/diff/29733600/sitescripts/extensi...
> sitescripts/extensions/bin/createNightlies.py:858:
> self.symlink_or_copy(self.path, linkPath)
> What if we don't have `self.path` because in `download_from_mozilla_addons` we
> followed the `else` branch of the `if` statement at line 549?

Good morning!

You are right about this creating a hidden dependency, howver 'self.path' is
used throughout the code in that way-ish after being set and then checked in
line 364 (and i wanted to keep it somewhat consistent).

Furthermore, there are 3 cases, implicitly covered by this hidden dependency

a) the download succeeds, self.path is set, we can actually use self.path.

b) the download didn't succeed due to a failed review (which would be the
else-branch in 588), self.path is not set (logging.error is called, try-finally
in 860 cleans things up since self.path is not set).

c) the download didn't succeed due to any other reason (from the code's point of
view), which in reality means that the review-process has not finished yet, we
do nothing but the try-finally in 860 still cleans things up.

(essentially, b and c are the answer to your inline-comment)

I do agree that there's much implicty involved - i could make this more
explicit, but IMHO returning the path from download_from_mozilla_addons() would
be inconsistent with the rest of the class.

[Vasily Kuznetsov, 2018-03-27 08:49:59]

On 2018/03/27 07:54:59, tlucas wrote:
> On 2018/03/26 19:12:12, Vasily Kuznetsov wrote:
> > Hi Tristan,
> > 
> > Looks good but the way you store the path in `self.path` (and then read it
> from
> > their later) instead of returning it explicitly makes me a bit uneasy. This
> > creates a hidden dependency between the code in
> `download_from_mozilla_addons()`
> > and the code in `download()` and can potentially break since not all paths
> > through `download_from_mozilla_addons` set `self.path`.
> > 
> > What do you think about returning the path instead of passing it through
> > `self.path`?
> > 
> > Cheers,
> > Vasily
> > 
> >
>
https://codereview.adblockplus.org/29733599/diff/29733600/sitescripts/extensi...
> > File sitescripts/extensions/bin/createNightlies.py (right):
> > 
> >
>
https://codereview.adblockplus.org/29733599/diff/29733600/sitescripts/extensi...
> > sitescripts/extensions/bin/createNightlies.py:858:
> > self.symlink_or_copy(self.path, linkPath)
> > What if we don't have `self.path` because in `download_from_mozilla_addons`
we
> > followed the `else` branch of the `if` statement at line 549?
> 
> Good morning!
> 
> You are right about this creating a hidden dependency, howver 'self.path' is
> used throughout the code in that way-ish after being set and then checked in
> line 364 (and i wanted to keep it somewhat consistent).
> 
> Furthermore, there are 3 cases, implicitly covered by this hidden dependency
> 
> a) the download succeeds, self.path is set, we can actually use self.path.
> 
> b) the download didn't succeed due to a failed review (which would be the
> else-branch in 588), self.path is not set (logging.error is called,
try-finally
> in 860 cleans things up since self.path is not set).
> 
> c) the download didn't succeed due to any other reason (from the code's point
of
> view), which in reality means that the review-process has not finished yet, we
> do nothing but the try-finally in 860 still cleans things up.
> 
> (essentially, b and c are the answer to your inline-comment)
> 
> I do agree that there's much implicty involved - i could make this more
> explicit, but IMHO returning the path from download_from_mozilla_addons()
would
> be inconsistent with the rest of the class.

Hi Tristan,

I see. So `self.path` is basically the path to the file that contains the result
of build or download. I agree with you that having similar logic in
download_from_mozilla_addons() and build() in terms of how this is stored makes
sense.

Still, when download_from_mozilla_addons() fails, this is not known to the code
of download() and we'll get an AttributeError on line 858. The exception will be
caught on line 887 and logged, but this error message doesn't seem very useful.
Would it perhaps be better to handle the case when
`download_from_mozilla_addons` doesn't download anything more gracefully?

Cheers,
Vasily

[tlucas, 2018-03-27 10:13:50]

Patch Set 2

 * "Fail" gracefully when no download happened

On 2018/03/27 08:49:59, Vasily Kuznetsov wrote:
> On 2018/03/27 07:54:59, tlucas wrote:
> > On 2018/03/26 19:12:12, Vasily Kuznetsov wrote:
> > > Hi Tristan,
> > > 
> > > Looks good but the way you store the path in `self.path` (and then read it
> > from
> > > their later) instead of returning it explicitly makes me a bit uneasy.
This
> > > creates a hidden dependency between the code in
> > `download_from_mozilla_addons()`
> > > and the code in `download()` and can potentially break since not all paths
> > > through `download_from_mozilla_addons` set `self.path`.
> > > 
> > > What do you think about returning the path instead of passing it through
> > > `self.path`?
> > > 
> > > Cheers,
> > > Vasily
> > > 
> > >
> >
>
https://codereview.adblockplus.org/29733599/diff/29733600/sitescripts/extensi...
> > > File sitescripts/extensions/bin/createNightlies.py (right):
> > > 
> > >
> >
>
https://codereview.adblockplus.org/29733599/diff/29733600/sitescripts/extensi...
> > > sitescripts/extensions/bin/createNightlies.py:858:
> > > self.symlink_or_copy(self.path, linkPath)
> > > What if we don't have `self.path` because in
`download_from_mozilla_addons`
> we
> > > followed the `else` branch of the `if` statement at line 549?
> > 
> > Good morning!
> > 
> > You are right about this creating a hidden dependency, howver 'self.path' is
> > used throughout the code in that way-ish after being set and then checked in
> > line 364 (and i wanted to keep it somewhat consistent).
> > 
> > Furthermore, there are 3 cases, implicitly covered by this hidden dependency
> > 
> > a) the download succeeds, self.path is set, we can actually use self.path.
> > 
> > b) the download didn't succeed due to a failed review (which would be the
> > else-branch in 588), self.path is not set (logging.error is called,
> try-finally
> > in 860 cleans things up since self.path is not set).
> > 
> > c) the download didn't succeed due to any other reason (from the code's
point
> of
> > view), which in reality means that the review-process has not finished yet,
we
> > do nothing but the try-finally in 860 still cleans things up.
> > 
> > (essentially, b and c are the answer to your inline-comment)
> > 
> > I do agree that there's much implicty involved - i could make this more
> > explicit, but IMHO returning the path from download_from_mozilla_addons()
> would
> > be inconsistent with the rest of the class.
> 
> Hi Tristan,
> 
> I see. So `self.path` is basically the path to the file that contains the
result
> of build or download. I agree with you that having similar logic in
> download_from_mozilla_addons() and build() in terms of how this is stored
makes
> sense.
> 
> Still, when download_from_mozilla_addons() fails, this is not known to the
code
> of download() and we'll get an AttributeError on line 858. The exception will
be
> caught on line 887 and logged, but this error message doesn't seem very
useful.
> Would it perhaps be better to handle the case when
> `download_from_mozilla_addons` doesn't download anything more gracefully?
> 
> Cheers,
> Vasily

Done ;)

[tlucas, 2018-03-27 10:28:12]

Patch Set 3

* Ensure existence of the attribute "self.path"

[Vasily Kuznetsov, 2018-03-27 10:29:38]

On 2018/03/27 10:28:12, tlucas wrote:
> Patch Set 3
> 
> * Ensure existence of the attribute "self.path"

LGTM

[Sebastian Noack, 2018-03-27 18:27:13]

LGTM