Relative file and path installation

[techalchemy]

• Issues - Package name can collide with top-level directory #949, Installing packages with relative paths fails -- discussion #939, Fix is_file #936 and to a lesser extent Pipenv does not support installing from local source packages #817 issues with relative packages #540 Cannot install from a local wheel #914 Installing TensorFlow from wheel fails #960 extras_require and length of name > 5 fails. #870 and more
• Fixed: Local file path installation (resolves in pipfile as relative
  path)
• Pass file:// URI to Requirements library for resolving

TODO:

• Ignore non-explicit directory paths lacking os.sep or ./
• Fix extras_require (per extras_require and length of name > 5 fails. #870)
• Add tests

[erinxocon]

We're gonna want @kennethreitz to look at this before merging, he wanted to take a look at the dependency resolution stuff before it was commited.

[techalchemy]

Yah it’s not anywhere near ready yet. I have about 5 more trains of thought I want to explore on this before we consider merging. Implementation details on resolving requirements working around the file URI, etc. This used to work without workarounds like this so I fear we merged a regression at some point. It might be simplest to start by figuring out where.

…

On Oct 24, 2017, at 12:19 AM, erinxocon ***@***.***> wrote: We're gonna want @kennethreitz to look at this before merging, he wanted to take a look at the dependency resolution stuff before it was commited. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[erinxocon]

@techalchemy let me know how I can help out here. You know more about this than I about this.

[techalchemy]

@erinxocon I think it might actually be a lot less complicated than I was thinking. I need to rethink this

[techalchemy]

Here was how we used to handle this: https://github.com/techalchemy/pipenv/blob/c7056ea4067c213e0702167f5207cc0eb45dea42/pipenv/cli.py#L836 -- this got reverted somewhere

[techalchemy]

ffffffffffffff i broke my own tests with this one, but it is much closer, more to come

[nateprewitt]

@techalchemy could we get a quick breakdown of what pathlib is buying us here? That's a pretty significant import.

[techalchemy]

@nateprewitt I imported it for cross-platform backward- and forward- path-to-uri conversions, consistent path resolution and normalization, etc. I was only importing Path specifically, but I vendored pathlib2 for backward compatibility and it may be lighter weight to import Path from there.

The longer-term plan was to refactor a lot of our path-handling functions and replace them with pathlib.Path since it does a better, more consistent job normalizing, resolving, and formatting across platforms

[techalchemy]

Appveyor test are off by a backslash, but as-is this code provides the same behavior as we have for file URIs

[techalchemy]

@nateprewitt @erinxocon @kennethreitz All tests are passing on this, and it fixes (currently broken) functionality for installing local files via relative paths. This was a regression and has caused a lot of issues (I've seen a number of threads on reddit and there have been mentions of reservations about pipenv related to inability to install local wheels, for example). This does not fix #870 and does not provide the thorough refactor of is_file which is needed, I will do that in a separate branch.

[nateprewitt]

Thanks @techalchemy, this on my list of reviews for tonight.

[techalchemy]

I do have a refactor of is_file that is now working, i will PR it separately when I have some tests. Slightly messier than I thought but not too bad

[nateprewitt]

Hey @techalchemy, thanks for all the work you're doing on this.

I've left some notes inline, some questions, other requests. Feel free to push back on or clarify anything I've commented on.

I've got some more stuff I want to address about our general approach to handling resource identifiers but I'm not going to get that typed up tonight. Hopefully this will get the discussion going a bit though.

[nateprewitt]

This also looks a bit odd to me. ../otherproject/file.zip should be an acceptable relative path.

[techalchemy]

the goal here was to make everything explicitly relative to the project path. I'm not 100% sure that is best, but it was the path of least resistance when I was writing the code so I figured it was a good starting point

[nateprewitt]

So I'm a little unsure about this test. It's generally good to have concrete expected values, otherwise we're only ensuring that os.path.join is just a broken in get_converted_relative_path as it is on it's own.

[techalchemy]

Yeah I agree, I was fishing for a solution that would accommodate windows which had a special caveat for python 2.7. I'm just going to write a separate one for windows.

[nateprewitt]

What are we envisioning this function being used for in the future? I'm inclined not to add it until it has a use.

[techalchemy]

uhhhh this is an artifact that is no longer needed. Will dig up the axe.

[nateprewitt]

Why can't we pass dep here? Haven't we already asserted it's str-like?

[nateprewitt]

On second pass, doesn't this exacerbate the problem with local packages vs pypi packages?

If I have a directory named ansible in my package, and specify that I was to install ansible, I would say the correct behaviour is to install from pypi. If I want to install from my local directory, it's up to the me to dictate that (./ansible).

With this approach, it looks like a pypi install will never be possible.

[techalchemy]

I think we can pass dep, I wrote this section in kind of a hurry which will explain the next piece of information.

I was looking at this briefly yesterday before I stopped working on this, but I think you're right about the behavior of ./ansible. That is to say, with this current code, pipenv install ansible will accurately install ansible from pypi actually, but it won't write it to the lockfile. I actually think I have a different approach to handle this in general though and I'm curious about whether it will work. I will update the PR shortly.

[nateprewitt]

We just modified the path here in the get_requirement function but we modify it again below. Is the path adding needed in the function? It looks like we don't use it in either of the two calls this is used.

[techalchemy]

This is the only one I need to push back on -- yes it is needed, and yes it is used. get_requirement takes a string and uses requirements.parse() to parse it. The root of the issue we had with handling relative paths is that requirements.parse() can only take either a requirement name (without any path separators, it must be a valid package name according to the pep standards) or a URI.

In the case of a relative path, we need to instantiate a Requirement object by converting any existing relative paths to their corresponding file:// URI. However, we want to store the relative path in the Pipfile, which only happens if Requirement.path is set and Requirement.uri is not set. According to the spec, the file property is for URIs, but we are writing path for the relative path. So we store the relative path to put in the req.path property, but we resolve the file:// uri for parsing Requirements:

        dep_path = Path(dep)
        if dep_path.exists() and not dep_path.is_absolute():
            path = get_converted_relative_path(dep)
            dep = dep_path.resolve().as_uri()
        ...
    if req.local_file and req.uri and not req.path and path:
        req.path = path
        req.uri = None
    return req

The modifications we are doing in convert_deps_from_pip don't have any impact to req.path in the cases where this code does. This code can surely be refactored, but the code I have runs only when the user supplies a local relative path (on reflection it should work on absolute paths also) whereas the other code modifies path only when there is a file supplied as the name, but there is no path or uri.

I basically left this code alone because there is only one conditional that possibly duplicates anything I've done and I didn't want to refactor the entire function which does quite a bit of heavy lifting, at least not in this PR

[techalchemy]

Hm I reevaluated some of this and in order to fix the ansible case above some of this needed to be refactored. Update incoming.

[techalchemy]

So there is some additional work to do on the file checks, but this fixes the ansible example (I didn't write a test for that yet) by using pip.utils.is_installable_dir for checking directories.

[techalchemy]

@nateprewitt This is currently functioning but I'm ok with including a refactor of some of the file related handling as well. I'm happy to have a more real-time discussion about the approach if you like

[kennethreitz]

I approve.

[kennethreitz]

✨🍰✨

[kennethreitz]

This is a terrible docstring :)

[nateprewitt]

@techalcyemy Alright, if we get the docstrig cleaned up, we can merge.

[nateprewitt]

@techalchemy can we squash this down to a commit or two?

[techalchemy]

@nateprewitt yes but completionism compels me to wait for CI

[techalchemy]

That squash was not fun but it is now done so feel free to review

[nateprewitt]

The failure on Windows 2.7 looks like it may be legitimate.

[techalchemy]

Uh yep. Might have missed something in the rebase. I’ll re-push when I get in

[ghost]

I happened to have this issue and I just tested the fix - pipenv install foo.tar.gz will use the new relative path mechanism, but if you run pipenv install while having foo.tar.gz listed in requirements.txt, the absolute path to foo.tar.gz will still get hardcoded in Pipfile.lock.