-
-
Notifications
You must be signed in to change notification settings - Fork 375
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
Install specific requirement from setup.cfg
for testing
#1263
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.
tl;dr - I like this as a general direction; my only comments are around bike-sheddy details.
Specifically - Do we anticipate an "env util" other than installing dev requirements? And - if we do end up with a need for another tool, would there be any reason that we need to cram it into this tool, rather than just adding a new one?
My suggestion - make this an install_requirement.py
script at the top level of the project (with a bunch of docs at the top of the file describing exactly why it exists). There's no reason it needs to be hidden in the test folder; and a single purpose "install a requirement" script is easier to explain than a generic "environment utils" script.
If I were going to go down the path of "adding features that I'm not sure we have a use case for", I might tweak the command line interface so that invocation was python -m install_extra dev tox
- i.e., install a specific requirement from the named section, possibly with python -m install_extra dev
would install all the dev requirements without installing the package itself.
If we really want to go down the rabbit hole, it might be worth publishing this as a standalone package on PyPI. If we treat it the same way we treat pip and setuptools (i.e., a tool that is allowed to be unpinned because main is always stable and compatible). We can then add it as a simple requirement, then use it as install_extra dev tox
. This same requirement exists on Toga, rubicon, etc... and I can imagine it being useful to others...
.github/workflows/ci.yml
Outdated
# We don't actually want to install briefcase; we just | ||
# want the dev extras so we have a known version of tox. | ||
python -m pip install $(ls dist/briefcase-*.whl)[dev] | ||
python tests/env_utils.py install-req tox |
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.
This will likely be opaque enough for first-time readers (or, lets be honest, me in 2 months :-) that it's worth retaining a comment so it's clear what this is doing.
9bbabd5
to
5548a26
Compare
I've implemented the basic recommendations for this to remain as a relatively simple script. However, as for usage such as Speaking of conclusions...I'm not really super enthused with a random script in the root of the project (although, the location isn't so much my hesitation as much as it's just a script that's divorced from everything else). So, I'm considering this still to be an interim step towards a more formalized tool that would ideally be packaged and put on PyPI. Although, my hesitation there is I'm not particularly a packaging expert and I feel like there's a lot of holes and missing functionality given all the permutations of Python packaging. I'll do some more research, though, and see how this shakes out. |
Running
I can empathise; however, from my perspective, it's a key piece of project infrastructure that's relatively easy to explain, so I can live with it.
Oh, thar absolutely be dragons in here for the general case :-) But we don't need to solve the general case ourselves. |
install_requirement/__main__.py
Outdated
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.
Just confirming this change was deliberate - putting this in a directory isn't required for a single file module - a single hello.py
can be invoked as python -m hello
.
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.
it was deliberate insofar as I was thinking it was necessary :) i didn't realize python -m
would work like that. I'll revert it back in that case.
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.
A couple of questions/clarifications, but this mostly makes sense.
requirement | ||
for requirement in project_requirements | ||
if any(requirement.name.startswith(req) for req in requested_requirements) | ||
] |
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.
Is there a reason you're doing a startswith
check, rather than a literal? This will be overenthusiastic for pytest/pytest-xdist case (install-req --extra dev pytest
installs both pytest
and pytest-xdist
).
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.
Right; that is the intended use-case since many testing dependencies tend to be named with a prefix that's an implied dependency. Or in the least, that's true for the pytest
and coverage
plugins we're using.
This type of functionality should probably be supported with a regex but I didn't want to add that complication.
Using a literal may be more expected and allows the behavior to be more explicit.
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.
Yeah - this is relying on a very specific naming convention - while this may be valid for a lot of packages, there's not guarantee that it will always be true. I can also imagine a possible situation where we literally just want pytest and no plugins, so explicit will always be better than implicit.
# We don't actually want to install briefcase; we just | ||
# want the dev extras so we have a known version of tox. | ||
python -m pip install $(ls dist/briefcase-*.whl)[dev] | ||
python -m pip install --upgrade setuptools build wheel |
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.
I'm not 100% clear on why build
and wheel
need to be on this list. Is this purely defensive, or is there a specific need?
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.
This script depends on build
and wheel
.
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.
/me facepalms. Of course - missed the imports at the top of the script.
…r the requirement
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.
Nice work - makes for a nice cleanup!
Changes
--extra
Notes
pip install --only-dep=tox briefcase
to ever be a thing seems very unlikelytox
has expressed interest in supporting something like thispip-tools
pip-compile --resolver=backtracking --extra dev -o - 2>/dev/null | grep '^tox='
@freakboy3742, this is mostly to gauge interest after our quick messages about this. Let me know what you think. At the end of the day, there isn't that much time spent installing all of Briefcase...but when you just need
coverage
ortox
, it seems like a little much. If this doesn't seem worth the squeeze, I'm fine moving on.PR Checklist: