Summary

Add `rbt install` command for installing dependencies.

Review Request #8294 — Created July 18, 2016 and submitted July 25, 2016, 5:46 p.m.

Information

Owner

david

Repository

RBTools

Branch

release-0.7.x

Bugs

Depends On

Commit

c06909a...

Reviewers

Groups

rbtools

People

Description

In some cases, we want to be able to install extra dependencies on-demand

instead of shipping everything with RBTools. The current example of this is

that we're pulling the TFS interface out into our own custom-written helper

(instead of making everyone download Team Explorer Everywhere), but it's too

big to just include in the egg.
With this new command, users can download and install said dependencies in one

command. This doesn't attempt to be a full-fledged package manager (there's no

versioning, or dependencies, or conflict resolution, etc), but it should be

good enough.
This does make some efforts to verify that the downloaded package is authentic.

If gpg is installed, it will check that the file is correctly signed.

Otherwise, it will check the sha256sum.

Testing Done

Used this a bunch while iterating on the TFS module. Checked that both
signature and sha256sum validation worked as expected.

Issues

Description	From	Last Updated
local variable 'gpg_dir' is assigned to but never used	reviewbot	July 18, 2016, 4:19 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	July 18, 2016, 4:51 p.m.
Swap these.	chipx86	July 20, 2016, 3:42 p.m.
"RBTools"	chipx86	July 20, 2016, 3:42 p.m.
Missing period.	chipx86	July 20, 2016, 3:42 p.m.
Let's place this in its own directory on downloads.beanbaginc.com. That'll make it easier for our stuff to manage the files.	chipx86	July 20, 2016, 4:05 p.m.
No need for the [].	chipx86	July 20, 2016, 3:42 p.m.
Period after sentences in comments, here and below.	chipx86	July 20, 2016, 3:42 p.m.
Trying to decide.... since these log entries (particularly the logging.info ones) are being output to the terminal, should we have …	chipx86	July 20, 2016, 3:43 p.m.
%s instead of +	chipx86	July 20, 2016, 3:44 p.m.
We should delete this before we raise CommandError above.	chipx86	July 20, 2016, 3:45 p.m.
Let's use %s instead of +. We also need to catch HTTPError and URLError for this. Particularly important when dealing …	chipx86	July 20, 2016, 3:45 p.m.
We should probably sanity-check this, and raise an error if we're not seeing what we expect. Might want to sanity-check …	chipx86	July 20, 2016, 3:57 p.m.
Let's put more information in here, say what hash we expected and what we got, specify the URL, and explain …	chipx86	July 20, 2016, 4:05 p.m.
We should either delete this when aborting above, or specify the location so that users can handle/refer to it.	chipx86	July 20, 2016, 3:53 p.m.
We should have try/except to catch any IOError or OSError from this.	chipx86	July 20, 2016, 4:05 p.m.
We need to catch here too, in case permissions are wrong (or something like SELinux gets in the way).	chipx86	July 20, 2016, 4:05 p.m.
Need to catch URLError and HTTPError here. (Or maybe the caller should do it? In which case, the docstring should …	chipx86	July 20, 2016, 4:05 p.m.
The description must be indented for exceptions.	chipx86	July 21, 2016, 10:18 a.m.
Can we wrap the line at the \n? We could maybe start the string indented 4 spaces from the raise, …	chipx86	July 21, 2016, 10:34 a.m.
Blank line between these.	chipx86	July 21, 2016, 10:20 a.m.
This can be while read_bytes != total_bytes:, instead of having that at the end.	chipx86	July 21, 2016, 10:20 a.m.
Needs the full module path.	chipx86	July 25, 2016, 5:09 p.m.
Newlines don't work so well in debug output. Subsequent lines don't get the prefix. Let's make each of these lines …	chipx86	July 25, 2016, 5:45 p.m.

Tool: Pyflakes
Processed Files:
    setup.py
    rbtools/commands/install.py



Tool: PEP8 Style Checker
Processed Files:
    setup.py
    rbtools/commands/install.py

rbtools/commands/install.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 local variable 'gpg_dir' is assigned to but never used
```

Commit:

1b6de96740421ff489ad9904757fd18fe573ae32

b42c51fe50bd3be0fcb2cd2a3d9226862bf3afdd

Diff:

Revision 2 (+165 -1)

Show changes

	setup.py
	rbtools/commands/install.py

Tool: Pyflakes
Processed Files:
    setup.py
    rbtools/commands/install.py



Tool: PEP8 Style Checker
Processed Files:
    setup.py
    rbtools/commands/install.py

rbtools/commands/install.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```

rbtools/commands/install.py (Diff revision 2)

Looking at other commands, it seems like this should be enclosed in brackets for consistency: <package>

Commit:

b42c51fe50bd3be0fcb2cd2a3d9226862bf3afdd

72987d09ba726a889e78b9579daac45450f8aca0

Diff:

Revision 3 (+166 -1)

Show changes

	setup.py
	rbtools/commands/install.py

Tool: PEP8 Style Checker
Processed Files:
    setup.py
    rbtools/commands/install.py



Tool: Pyflakes
Processed Files:
    setup.py
    rbtools/commands/install.py

rbtools/commands/install.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Swap these.
```
rbtools/commands/install.py (Diff revision 3)
The issue has been resolved. Show all issues
```
"RBTools"
```
rbtools/commands/install.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing period.
```

rbtools/commands/install.py (Diff revision 3)

The issue has been resolved. Show all issues

Let's place this in its own directory on downloads.beanbaginc.com. That'll make it easier for our stuff to manage the files.

rbtools/commands/install.py (Diff revision 3)
The issue has been resolved. Show all issues
```
No need for the [].
```
rbtools/commands/install.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Period after sentences in comments, here and below.
```

rbtools/commands/install.py (Diff revision 3)

The issue has been resolved. Show all issues

Trying to decide.... since these log entries (particularly the logging.info ones) are being output to the terminal, should we have these in sentence format, with a period at the end?

RBTools logging.info and CommandError-based output generally do unless doing My message: %s. I kinda feel that'd be the right thing to do.

david July 20, 2016, 4:07 p.m.

I'll do it for the ones that print but not the logging.debug ones.

rbtools/commands/install.py (Diff revision 3)
The issue has been resolved. Show all issues
```
%s instead of +
```
rbtools/commands/install.py (Diff revision 3)
The issue has been resolved. Show all issues
```
We should delete this before we raise CommandError above.
```

rbtools/commands/install.py (Diff revision 3)

The issue has been resolved. Show all issues

Let's use %s instead of +.

We also need to catch HTTPError and URLError for this. Particularly important when dealing with proxies.

Should we use download_file for this?

david July 20, 2016, 4:07 p.m.

I initially used download_file but it's overkill since we really don't care about having the file saved to disk and then we have to delete it.

rbtools/commands/install.py (Diff revision 3)

The issue has been dropped. Show all issues

We should probably sanity-check this, and raise an error if we're not seeing what we expect.

Might want to sanity-check the length of the real_sha as well.

david July 20, 2016, 4:07 p.m.

I don't think we really need to check it here because if it's not what we expect, it won't match the sum that we compute below, and we'll show an error there.

rbtools/commands/install.py (Diff revision 3)

The issue has been resolved. Show all issues

Let's put more information in here, say what hash we expected and what we got, specify the URL, and explain what this means (the file might not be safe).

david July 20, 2016, 4:07 p.m.

I'm not sure spitting out two 64-character hashes is really useful at all to anyone.

rbtools/commands/install.py (Diff revision 3)

The issue has been resolved. Show all issues

We should either delete this when aborting above, or specify the location so that users can handle/refer to it.

rbtools/commands/install.py (Diff revision 3)

The issue has been resolved. Show all issues

We should have try/except to catch any IOError or OSError from this.

rbtools/commands/install.py (Diff revision 3)

The issue has been resolved. Show all issues

We need to catch here too, in case permissions are wrong (or something like SELinux gets in the way).

rbtools/commands/install.py (Diff revision 3)

The issue has been resolved. Show all issues

Need to catch URLError and HTTPError here. (Or maybe the caller should do it? In which case, the docstring should specify that those can raise.)

Commit:

72987d09ba726a889e78b9579daac45450f8aca0

67f04199cfbc67548699e48c7ca93000d46a340e

Diff:

Revision 4 (+207 -1)

Show changes

	setup.py
	rbtools/commands/install.py

Tool: Pyflakes
Processed Files:
    setup.py
    rbtools/commands/install.py



Tool: PEP8 Style Checker
Processed Files:
    setup.py
    rbtools/commands/install.py

rbtools/commands/install.py (Diff revision 4)
The issue has been resolved. Show all issues
```
The description must be indented for exceptions.
```

rbtools/commands/install.py (Diff revision 4)

The issue has been resolved. Show all issues

Can we wrap the line at the \n? We could maybe start the string indented 4 spaces from the raise, like above.

It'd also be nice to specify the signatures (expected and actual), so the user can compare themselves if needed, and report to us. Would particularly help if there's an issue of some sort with the file contents from an upload.

david July 21, 2016, 10:22 a.m.

I don't really think it's that useful to spew out 128 characters of junk, especially since the files themselves will have been removed after the command exits. A user "comparing it themselves" will say "yep, those are different" but it won't mean anything and there won't be anything they can do about it.

If there's a problem with the upload, there's no difference between them reporting it to us, and I would hope that we'd test the installation any time we upload the files anyway.

chipx86 July 25, 2016, 4:41 p.m.

It'd be nice to have something that we can inspect, at least. Perhaps log output showing the SHAs + pointing to the file paths in a temp directory, so that we can have them send us data on them.

Plenty of things can go wrong that aren't necessarily our fault. Proxy issues causing bad content, some S3 error payload due to something going wrong, MITM (which would be useful to inspect). If something does go wrong, we should know about it, and we'll need useful information to help with that.

What we could do is log the SHAs and the file paths as a CRITICAL and, in the error message, say to report this to us along with the contents of the files mentioned.

david July 25, 2016, 4:50 p.m.

Are you saying you want me to undo the work to ensure things are deleted on cleanup?

chipx86 July 25, 2016, 4:59 p.m.

Hmm.. Kinda contradictory, isn't it... No, I think the work was good and should be kept, and takes care of the other cases where things might fail but state was left behind.

I guess we can always have them manually download the file in a browser or something if we need to debug it. It'd still be good to have the SHAs in the debug log so we can verify that RBTools is getting the same thing the browser's getting (helping to diagnose proxy issues or something). So I guess my only ask would be including the SHAs in the debug output.

rbtools/commands/install.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Blank line between these.
```

rbtools/commands/install.py (Diff revision 4)

The issue has been resolved. Show all issues

This can be while read_bytes != total_bytes:, instead of having that at the end.

Commit:

67f04199cfbc67548699e48c7ca93000d46a340e

fe9ffbef1b84975c21b0f9acaee7c11622164440

Diff:

Revision 5 (+206 -1)

Show changes

	setup.py
	rbtools/commands/install.py

Tool: PEP8 Style Checker
Processed Files:
    setup.py
    rbtools/commands/install.py



Tool: Pyflakes
Processed Files:
    setup.py
    rbtools/commands/install.py

rbtools/commands/install.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Needs the full module path.
```

Change Summary:

Add debug log of SHA checks, fix up documentation on exceptions raised.

Commit:

fe9ffbef1b84975c21b0f9acaee7c11622164440

c06909a82841b996ea030c10a00c7ea5b267b074

Diff:

Revision 6 (+222 -1)

Show changes

	setup.py
	rbtools/commands/install.py

Tool: PEP8 Style Checker
Processed Files:
    setup.py
    rbtools/commands/install.py



Tool: Pyflakes
Processed Files:
    setup.py
    rbtools/commands/install.py

Ship it!

rbtools/commands/install.py (Diff revisions 5 - 6)

The issue has been resolved. Show all issues

Newlines don't work so well in debug output. Subsequent lines don't get the prefix. Let's make each of these lines a logging.debug().

Status:: Completed
Change Summary:: Pushed to release-0.7.x (a84c507)