-
Notifications
You must be signed in to change notification settings - Fork 134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Put runtime dependencies in install_requires (fixes #308) #309
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
9d9d911
to
0b2e6b3
Compare
Codecov Report
@@ Coverage Diff @@
## master #309 +/- ##
=======================================
Coverage 96.31% 96.31%
=======================================
Files 11 11
Lines 814 814
Branches 100 100
=======================================
Hits 784 784
Misses 10 10
Partials 20 20 Continue to review full report at Codecov.
|
Signed-off-by: Patrick Decat <[email protected]>
0b2e6b3
to
b7d4ad3
Compare
I guess the following comment is obsolete:
|
FWIW, same checks with python3 and venv. Without this change:
With this change:
|
Thanks very much for debugging this and providing a fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hrm doesn't this changeset just mean that you've pushed the ContextualversionConflict problem from setup to install time? From the virtualenv test run, it looks like the only reason we missed hitting this same problem was because of the inability to install pbr in the virtualenv per the warning
/usr/lib/python2.7/distutils/dist.py:267: UserWarning: Unknown distribution option: 'pbr'
warnings.warn(msg)
If we had been able to install, wouldn't that have replicated the same error because pbr would have pulled in latest urllib3 @ 1.23?
At install time, pip will properly resolve runtime dependencies. |
On second thought, perhaps the |
Just tested that, it removes the I'm opening a second PR. |
…anonical#309) Signed-off-by: Patrick Decat <[email protected]>
@blackboxsw and @pdecat thanks for pursuing this to the relentless end. 👍 |
Signed-off-by: Patrick Decat <[email protected]>
…anonical#309) Signed-off-by: Patrick Decat <[email protected]>
…anonical#309) (canonical#310) Signed-off-by: Patrick Decat <[email protected]>
Fixes #308
Without this change:
and
and
With this change:
and
and