-
Notifications
You must be signed in to change notification settings - Fork 104
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
Support pypi repository on the local disk (file://
urls)
#538
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for conda-lock ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
The code is not really nice, I especially dislike that I had to basically copy the code for At least the required changes to requests-file are clearly visible as I created two commits. |
Thanks a lot for this!
At the absolute level maybe not, but given the approach this seems pretty good to me at first glance. Regarding the changes to It may take me some days to give this a proper review. I'd also request a few tests, but those could wait until after the first review in case you think we might come up with a better approach. |
I thought a bit about this but in the end I can not think of a better approach unless we write a Repository implementation from scratch. Maybe you can indicate some tests you would like to see? I expect something like create some static pypi html in a temp directory + a private dummy package and check that conda-lock can install it? I guess the |
That all sounds good. I don't expect this to be a common use-case, but for robustness it'd be nice to have a test which exercises both http and file repos at the same time. |
@maresb I added a simple test case so I hope you can start a review, maybe we can get this in soon-ish? I guess there will be quite a few things that need to be polished, but let's hope nothing too serious. I created a new test file because the html has subtle differences but I dislike that there is a bit of duplication with the original pypi repo test. If you have some ideas for improvments, I am all ears. |
Moin @FelixSchwarz! Sorry to take so long to come back to this. I had a look and it seems very good. I did a pass of making some changes to make things a bit more logical in my mind, but don't be shy about reverting anything you don't like, and let me know if there's something you think I misunderstood. When I run the test locally it freezes without timing out while trying to access https://pypi.org/pypi/six/json. I'm not sure what that's about, but it's a bit unsettling. Hopefully works in CI. |
Oh, I forgot to mention: could you please try opening a PR in requests-file for this? That would be a lot better than the current vendoring solution. |
Thank you for improving my PR. I'll create a PR for requests-file, just give me a few days. In CI there was a test failure related to mamba on Windows. I saw that micromamba tests are skipped there but I guess mamba should work? If so, I need to dust of a Windows VM and try running the tests there. Do I need some special setup on Windows to run the tests (e.g. do I really need to install docker?) |
Ya, that failure seems quite odd. I'm rerunning the test in hopes that we get lucky.
Windows is always a pain to debug. You could try debugging through GitHub, but that's slow and non-interactive. Marius disabled a bunch of the tests because they were slow and flaky, but we should probably reenable them. You shouldn't need to run Docker, and really the most important test to get running is yours. So if there's a windows-specific problem with your test and you fix it in a VM and then everything passes in CI I think that's good enough. Ah, I also wanted to ask you how this handles special characters like spaces and %. Is there any URL-decoding? (I can't think of any situations where the input needs to be encoded, so perhaps it's best to leave out decoding?) It may be nice to add some tightly-scoped unit test (without solving or with trivial solving) to verify that a dummy package can be located if it lives in a path with a difficult name. Since the user has control over where to place the packages, I don't think we should worry about crazy cases like a directory literally named Thanks for your work on this, I think it's a really valuable feature! |
Still seeing the same failure after rerunning. :( |
I just checked again and the problem is this: requests_file uses I can adapt the PR but I'm not sure what to do here - also I don't want to spend too much time fixing something just for Windows (which I do not use). |
Wow, thanks so much @FelixSchwarz for the deep dive on Windows. I share your apathy for that platform, but we have users depending on it. I just checked and |
This is a quick PR to support
file://
urls which resolves #536 .Since #529 there is support for additional pip repositories. With this PR also local repositories are supported, specified by a
file://
url like this:Why is this needed?
We require some private wheels for our repo but there is no private registry in place. The easiest thing for us seems to be to put the wheels in git lfs which is available for every developer. That way we do not need to operate a private pypi service and we circumvent all the potential problems with regards to authentication/user accounts.
Running
conda-lock
with afile://
url triggers an exception by requests:requests.exceptions.InvalidSchema: No connection adapters were found for 'file:///some/path/to//'
.As a follow-up I will submit a patch to the requests-file project but only if conda-lock actually needs it.