Skip to content
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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

guysalt
Copy link

@guysalt guysalt commented May 4, 2024

closes #1389

  • I have added a news fragment under changelog.d/ (if the patch affects the end users)

Summary of changes

Fix error with pipx install Command Using Relative Path for Pip Arguments

Test plan

Tested by running

pipx install --pip-args=--constraint=some_constraint_file.txt some_package_inside_constraint_file

@guysalt guysalt changed the title Fix error with pipx install command using --pip-args (#1389) Fix error with pipx install command using --pip-args May 4, 2024
Copy link
Contributor

@chrysle chrysle left a 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.

src/pipx/package_specifier.py Outdated Show resolved Hide resolved
src/pipx/package_specifier.py Outdated Show resolved Hide resolved
changelog.d/1389.bugfix.md Outdated Show resolved Hide resolved
src/pipx/package_specifier.py Outdated Show resolved Hide resolved
@guysalt
Copy link
Author

guysalt commented May 4, 2024

Thanks for catching this! Please also extend the docstring of the changed function.

I have considered this * Convert local paths to absolute paths as the same description, doesn't it?

@guysalt guysalt force-pushed the main branch 2 times, most recently from 47f4b58 to 138fa87 Compare May 4, 2024 14:14
@guysalt guysalt force-pushed the main branch 3 times, most recently from 6bd33c5 to d9d8663 Compare May 4, 2024 23:32
@guysalt guysalt changed the title Fix error with pipx install command using --pip-args Fix passing constraints file path into pipx install operation via --pip-args May 4, 2024
@chrysle
Copy link
Contributor

chrysle commented May 7, 2024

Great, let's get down to business once again, then. Don't fail to inform us about your XONSH compatibility research.
Could you add a functional test, please?

@guysalt
Copy link
Author

guysalt commented May 7, 2024

Great, let's get down to business once again, then. Don't fail to inform us about your XONSH compatibility research. Could you add a functional test, please?

sure, hope to do so soon...

@guysalt guysalt force-pushed the main branch 2 times, most recently from 677d47b to 919d7e9 Compare May 7, 2024 18:42
@guysalt
Copy link
Author

guysalt commented May 7, 2024

Great, let's get down to business once again, then. Don't fail to inform us about your XONSH compatibility research. Could you add a functional test, please?

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.
first I wrote a test just for checking the path is absolute and the command working (with empty constraint file).

then I want to add a check for installing the right constraint from the file.
when I put in the constraint file pycowsay==0.0.0.1 it does not installing the package (says something about conflict).

and if I run similar command on my terminal its working, so im trying to understand what is the different?
maybe you can help me understand its related to the tests architecture or something like this.

@guysalt
Copy link
Author

guysalt commented May 8, 2024

Great, let's get down to business once again, then. Don't fail to inform us about your XONSH compatibility research. Could you add a functional test, please?

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. first I wrote a test just for checking the path is absolute and the command working (with empty constraint file).

then I want to add a check for installing the right constraint from the file. when I put in the constraint file pycowsay==0.0.0.1 it does not installing the package (says something about conflict).

and if I run similar command on my terminal its working, so im trying to understand what is the different? maybe you can help me understand its related to the tests architecture or something like this.

understand the catch, add pycowsay==0.0.0.1 to the tests_packages files...
need to see all the tests pass

chrysle
chrysle previously approved these changes May 8, 2024
Copy link
Contributor

@chrysle chrysle left a 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 Show resolved Hide resolved
tests/test_install.py Outdated Show resolved Hide resolved
tests/test_install.py Outdated Show resolved Hide resolved
testdata/tests_packages/unix-python3.10.txt Outdated Show resolved Hide resolved
tests/test_install.py Outdated Show resolved Hide resolved
tests/test_install.py Outdated Show resolved Hide resolved
tests/test_install.py Outdated Show resolved Hide resolved
tests/test_install.py Outdated Show resolved Hide resolved
testdata/tests_packages/unix-python3.10.txt Outdated Show resolved Hide resolved
src/pipx/package_specifier.py Outdated Show resolved Hide resolved
src/pipx/package_specifier.py Outdated Show resolved Hide resolved
@guysalt guysalt force-pushed the main branch 4 times, most recently from ec8ed19 to d45a72e Compare May 14, 2024 16:35
@guysalt guysalt force-pushed the main branch 2 times, most recently from c2764bb to d77d362 Compare May 15, 2024 09:55
tests/test_install.py Show resolved Hide resolved
tests/test_install.py Outdated Show resolved Hide resolved
@guysalt guysalt force-pushed the main branch 4 times, most recently from 45b925b to cc6a26f Compare May 18, 2024 12:01
@guysalt
Copy link
Author

guysalt commented May 19, 2024

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?

@guysalt
Copy link
Author

guysalt commented Jun 25, 2024

Hey again @chrysle

I was a little bit busy, I would like to keep going and finish this please.

We discussed about that thing with the ' on the first thread here.
Now, after I add the last commit that strip the ' char, all the tests are working...

Please review it, and let me know if you still don't want the striping on the pip args.
Thanks

@chrysle
Copy link
Contributor

chrysle commented Jun 28, 2024

Looks good. Please add a comment to the code explaining why it is necessary, then I think this is mergeable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants