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.

Sign up for GitHub

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

Jump to bottom

Local dependency path resolution can fail on *nix #266

Closed
tymokvo opened this issue Feb 16, 2021 · 8 comments · Fixed by #268
Closed

Local dependency path resolution can fail on *nix #266

tymokvo opened this issue Feb 16, 2021 · 8 comments · Fixed by #268
Labels
bug Something isn't working released

Comments

@tymokvo
Copy link
Contributor

tymokvo commented Feb 16, 2021

Steps to reproduce

  1. Create new virtual env and activate
  2. Install a local pollination-dsl dependency
  3. Run pollination dsl translate <name of recipe using local dependency> <some local path>

Expect:
Command fails with:

urllib.error.URLError: <urlopen error [Errno 2] No such file or directory: '//<current working directory>/<absolute path to index.json>'>

This appears to come from this line where the assumption is made that, if the path starts with file://, then it is relative to the current working directory.
@mostaphaRoudsari mostaphaRoudsari added the bug Something isn't working label Feb 16, 2021
@mostaphaRoudsari
Copy link
Member

Hi @tymokvo, thank you for documenting this. How important is resolving this issue for finishing the other PR for updating the docs?

@tymokvo
Copy link
Contributor Author

tymokvo commented Feb 17, 2021

Medium important, I guess. I can't really use the example that you put together until the translate command works. But I can merge the PR without the example and update the docs once I get it working if that is ok.

@tymokvo
Copy link
Contributor Author

tymokvo commented Feb 17, 2021

So I've been digging into this a bit today and I think the issue stems from the use of .split('file:///') on source URLs in multiple places in the code base. I think the best option might be to replace this pattern with urlparse(source) and then handle the case the parse result has file as its scheme property. What do you think?

@mostaphaRoudsari
Copy link
Member

mostaphaRoudsari commented Feb 17, 2021 •
edited
Loading

If that works then I have nothing against it. urlparse was not great in handling local files and that why the .split('file:///') started to show up as an alternative solution to handle the case for local registry. I doubt this will resolve the cwd issue that you pointed above but if it does then even better! 😀

related -> c69a7fe

@tymokvo
Copy link
Contributor Author

tymokvo commented Feb 17, 2021

Ugh, windows. Ok, well I will try a different solution, then.

@mostaphaRoudsari
Copy link
Member

It worked fine for a plugin:

image

but I could create the error for a recipe. I'll give it a try later today and will report back.

mostaphaRoudsari added a commit to mostaphaRoudsari/queenbee that referenced this issue Feb 18, 2021
@mostaphaRoudsari
fix: resolve local dependency path on \*nix correctly
19e35a0 
This PR addresses the issue with resolving the local path as a uri. It took mixed usage of `os.path` and `pathlib.Path` to get it right on both Windows and Linux. Part of the issue was how in Windows and \*nix systems based on what the uri is pointing to the number of slashes after file:// can be 2 or 3! I was expecting it to always be 3 which would return `home/user/...` instead of `/home/user/...`. The first one is considered an absolute path and the second one is relative! That's the main reason that it was failing. I also took the opportunity to clean the code a little bit.

resolves pollination#266
@mostaphaRoudsari mostaphaRoudsari mentioned this issue Feb 18, 2021
Merged
@mostaphaRoudsari
Copy link
Member

Hi @tymokvo, this PR should resolve the issue on your side: (#268). I asked for your review. 🤞

mostaphaRoudsari added a commit that referenced this issue Feb 18, 2021
@mostaphaRoudsari
fix: resolve local dependency path on \*nix correctly
baf3997 
This PR addresses the issue with resolving the local path as a uri. It took mixed usage of `os.path` and `pathlib.Path` to get it right on both Windows and Linux. Part of the issue was how in Windows and \*nix systems based on what the uri is pointing to the number of slashes after file:// can be 2 or 3! I was expecting it to always be 3 which would return `home/user/...` instead of `/home/user/...`. The first one is considered an absolute path and the second one is relative! That's the main reason that it was failing. I also took the opportunity to clean the code a little bit.

resolves #266
@ladybugbot
Copy link

🎉 This issue has been resolved in version 1.23.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working released
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: resolve local dependency path on \*nix correctly mostaphaRoudsari/queenbee
3 participants
@mostaphaRoudsari @tymokvo @ladybugbot