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 an issue for projects with an at sign (@) in the path #310

Merged
merged 6 commits into from
Jun 25, 2022

Conversation

jervi
Copy link
Contributor

@jervi jervi commented Mar 8, 2022

It seems requirementslib is causing a crash in pipenv in some rare circumstances. If you define a package like this in a Pipfile:

myproject = {editable = true, path = "."}

and this project path contains an at sign (@), then requirementslib will parse the path wrong. The second clause in the added test case will fail without this fix.

Fixes #309

@jervi jervi changed the title Fix an issue for projects with an at sign (@) in the path Fix an issue for projects with an at sign (@) in the path Mar 8, 2022
@matteius
Copy link
Collaborator

Thanks @jervi -- I have a fix for the test CI failing in my PR but it requires a new version of pip-shims for my other changes. I'll be able to fix that later this week and then I'll have you merge the updated main in your branch and then the tests should be runnable.

@matteius
Copy link
Collaborator

@jervi I have merged my changes into main -- if you can merge the latest main into your branch it should pass the next test run.

It seems `requirementslib` is causing a crash in `pipenv` in some rare circumstances. If you define a package like this in a `Pipfile`:
```
myproject = {editable = true, path = "."}
```
and this project path contains an at sign (`@`), then requirementslib will parse the path wrong. The second clause in the added test case will fail without this fix.

Fixes sarugaku#309
@jervi
Copy link
Contributor Author

jervi commented Mar 31, 2022

Thanks @matteius, I've rebased the branch now

@matteius
Copy link
Collaborator

@jervi it looks like the one failing test is for a VCS version which is a supported use case, so I think we have to sort out how to tell if the directory includes an @ or if the end of the path is specifying a vcs repository that is specifying the checkout version.

@jervi
Copy link
Contributor Author

jervi commented Mar 31, 2022

Hmm. I thought all the tests succeeded locally. I'll have another look.

@matteius
Copy link
Collaborator

@jervi I think that test does not run on Windows though.

@matteius
Copy link
Collaborator

matteius commented Apr 1, 2022

@jervi let me know what your timeline is like; I'd like to do a release of this library in say a week or sooner if this gets finalized. My rationale for a release soon is to get the new version of pip vendored into pipenv into a release for later in April.

@jervi
Copy link
Contributor Author

jervi commented Apr 7, 2022

Sorry, I meant to fix this during the last weekend, but things came up. I'll try to fix the test shortly - today or tomorrow 👍

The motivation is to only care about @ that is not part of the path but rather used for git refs
@jervi
Copy link
Contributor Author

jervi commented Apr 26, 2022

Long days these days 😆 I'm really sorry for the long delay. It's a bit cumbersome that the tests don't run automatically, so I can't really confirm, but I think it's working now.

@jervi
Copy link
Contributor Author

jervi commented Apr 26, 2022

What is a valid git ref in this context? Is it only tags or sha's? Or can it be branch names as well? This implementation will probably break if one are using a branch name for the ref and this branch contains a slash. I might need to adjust that regex a bit more...

@jervi
Copy link
Contributor Author

jervi commented Apr 26, 2022

Figured slashes in the ref names ought to be supported. This approach is maybe a bit naïve, but it's better than the previous attempt. All the existing tests works, and I added another test case that also works.

@nnutter
Copy link

nnutter commented Jun 24, 2022

I'm being hit by the bug (#309) this is trying to fix. Is there anything blocking this from being merged?

@matteius
Copy link
Collaborator

@nnutter Just that I want more reviews and confidence that its not going to break something :-) If you think it looks good, that helps me feel better about hitting the merge button. I can try to get this into a release soon.

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

Successfully merging this pull request may close these issues.

Support for paths with at sign (@)
3 participants