-
Notifications
You must be signed in to change notification settings - Fork 385
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
Fix passing constraints file path into pipx install
operation via --pip-args
#1390
base: main
Are you sure you want to change the base?
Conversation
pipx install
command using --pip-args
(#1389)pipx install
command using --pip-args
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.
Thanks for catching this! Please also extend the docstring of the changed function.
I have considered this |
47f4b58
to
138fa87
Compare
6bd33c5
to
d9d8663
Compare
pipx install
command using --pip-args
pipx install
operation via --pip-args
Great, let's get down to business once again, then. Don't fail to inform us about your XONSH compatibility research. |
sure, hope to do so soon... |
677d47b
to
919d7e9
Compare
hey, so I wrote a test, but i will glad if you help me with a question... the test right now is not passing and I dont understand why. then I want to add a check for installing the right constraint from the file. and if I run similar command on my terminal its working, so im trying to understand what is the different? |
understand the catch, add |
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.
Thanks for adding the tests.
tests/test_install.py
Outdated
|
||
temp_file = current_directory / constraint_file_name | ||
temp_file.touch() | ||
temp_file.write_text(f"pycowsay=={pycowsay_version}") |
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.
We have a small database for our test packages, which we use in tests.
temp_file.write_text(f"pycowsay=={pycowsay_version}") | |
temp_file.write_text(PKG["ipython"]["spec"]) |
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.
@guysalt I think you overlooked this comment?
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.
Also here, if someone change to the newer version the test will not check if the older versions indeed installed.
Changed
tests/test_install.py
Outdated
subprocess_pycowsay_available_versions = subprocess.run( | ||
[sys.executable, "-m", "pip", "index", "versions", "pycowsay"], capture_output=True, text=True, check=False | ||
) | ||
subprocess_pycowsay_available_versions_output = subprocess_pycowsay_available_versions.stdout | ||
output_lines = subprocess_pycowsay_available_versions_output.split("\n") | ||
available_versions_line = next(line for line in output_lines if "Available versions:" in line) | ||
versions_string = available_versions_line.split(":")[1].strip() | ||
available_versions = versions_string.split(", ") | ||
assert pycowsay_version in available_versions |
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.
Why do we need this? Besides, pip index
is still experimental.
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.
For double check if the required package version is available and if not, will be more clearly to say why the test fails.
do you prefer to remove this block?
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.
Hmm, if it wasn't available, there would be something wrong with our test packages setup. I'd tend not to over-complicate things.
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.
Ok, just wanted to check the setup for the tests good, and we have two different versions available on the tests and see the constraint flag works and the older version is installed.
Now after I wrote this I also see I did not check there at lease newer version indeed...
Anyway, Im ok also with not checking this,
Removed.
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.
Ok, just wanted to check the setup for the tests good, and we have two different versions available on the tests and see the constraint flag works and the older version is installed.
Although I think we can trust pip
it handles the constraints files it is given correctly (if it does find them), creating a constraints file without a proper constraint doesn't seem good. Could you try specifying a version not equal (!=
) to the spec in the constraints file? That way, we should be able to determine whether the mechanism works.
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.
ok now I got you, to do so with the PKG[package_name]["spec"]
or with const?
I prefer const as the PKG[package_name]["spec"]
taking the older version, and with const I can put not equal !=
to the newer version.
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 prefer const as the
PKG[package_name]["spec"]
taking the older version, and with const I can put not equal!=
to the newer version.
You could run a replace on that, but either way is fine.
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.
Now I see the newer version isn't the same in all the tests_packages
constraint files 😪 ...
How do I run a replace? Can I change it manually?
What do you think about put in the constraint file <
from PKG[package_name]["spec"]
and then check the version is indeed less then the PKG[package_name]["spec"]
.
For this I need that every test constraint file will contain at lease one version less the PKG[package_name]["spec"]
version.
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.
How do I run a replace? Can I change it manually?
I thought of just using the str
types' replace()
function?
As far as I'm aware, we don't even need to test whether the version after installing with constraints is less than the spec given; only if it is not equal? Sorry for the hassle here.
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.
Hey im sorry for the delay, yea im sorry for the hassle too lets finish it.
I wanted to check if it less than the spec given, to validate for sure the constraint file is working. as I mention before, even if I using not equal sign and the constraint file somehow does not working, the test still can pass if the pypi versions matching the constraint.
Anyway, if its ok for you its ok for me, so now I believe I did it as you asked for.
thanks for your time!
ec8ed19
to
d45a72e
Compare
c2764bb
to
d77d362
Compare
dbe79eb
to
45b925b
Compare
Its seems that the tests fails exactly on what I was trying to tell you on the pip args split... that the splitting does now working on the workflow, as it does now working on my terminal (xonsh). do you have a clue why does it happening? |
closes #1389
changelog.d/
(if the patch affects the end users)Summary of changes
Fix error with
pipx install
Command Using Relative Path for Pip ArgumentsTest plan
Tested by running