Add tests

[techalchemy]

Another piece of #55 plus #56 (2/4)

[techalchemy]

@uranusjr I think this can use a look, the PR says it is adding tests but there is a bit more going on -- significantly

• There is some refactor code bleeding over since I believe I missed a commit or two on my rebase of the other PR (which is merged now) -- you will likely want to look that over at some point, I am pretty confident you will find some abstractions you want to do there when you have time anyway
• There are some more serious changes which introduce fixes for things uncovered in test breakages / additional functionality which was missing before including:
  • Finalized implementation of Sdist resolution using the fallback metadata discovery you implemented as a wheel building failover
  • Actual installation of sdists, but invoking setup.py directly in a subprocess call which seems to be a bit more reliable assuming we have prepared the requirement
  • Note that once we can sort out the details in this PR, the next PR I will take a look at and clean up will be Split packages #58 which has these implementations split out into external packages already (you are a full owner on those)
  • I.e. if we can sort out how this should work, we can then make sure it aligns with the other libraries and just cut it out from here for now
• The main change is to the structure of what was the WheelInstaller class, which now ultimately lands in the Installer class -- there are a few reasons for that change, but largely it's because wheels need a way to try to install themselves as sdists if they fail in wheel form (I think the base class isn't named properly but that can be fixed)

Also note I have a venv argument going around in a lot of places to make this virtualenv aware, it seems kind of important that we have some way of being aware of that but the approach I took is mainly for installation and will be able to live specifically in the installer package. If you have some thoughts about the implementation I am interested in that for sure.

I have no real comments about the tests other than that they work, they use the virtualenv library I set up specifically for this purpose, we have about 65% code coverage (but not very thorough edge case testing) with this setup.

Sorry for the long comments, but there is a lot of effort into the PRs and I know you're kind of busy, so I wanted to have things somewhat cleaned up before dragging you into it. Happy to hold off merging anything that will introduce substantial changes until you can give it a good look over

[uranusjr]

Most of the things seem good to me (I didn’t read the changes to .gitignore; I assume they come from GitHub’s recommendation or somewhere). However, I don’t feel comfortable Passa needs to depend on Mork. I still intend Passa to be run strictly inside the target environment, and virtual environment management does not belong here. It is fine to have the ability to install into an environment different from the one Passa is run in, but that should be provided by the user (e.g. a custom prefix than sys.prefix) instead.

---

Edit: I read my comments again, and felt my wording is a bit off. The VenvInstaller feature is good in general, but I don’t like that it hard-depends on Mork, and Mork hard-depends on virtualenv. Pass should not require virtualenv.

[uranusjr]

Also, the Venv design doesn’t feel natural to me. Instead of checking “am I in a venv, or is the venv is None which means this is global”, the project should just take an abstract idea of an “environment”, and use it. Whether that environment is virtual or global or even something else is irrelevant to it; it just needs to know where the interpreter is, where to install purelib, where to install platlib, where to install scripts, etc.

[uranusjr]

I don’t think this is needed.

[techalchemy]

That would be correct, will fix

[uranusjr]

Why?

[techalchemy]

This is necessary to patch distlib in order to read metadata from version 2.1 for now

[uranusjr]

Comment needed too

[techalchemy]

Not sure if that ever got fixed but we can patch these manually. The library in question just does some runtime patching by looking for modules and swapping out functions

[techalchemy]

I don’t feel comfortable Passa needs to depend on Mork. I still intend Passa to be run strictly inside the target environment, and virtual environment management does not belong here.

Agreed -- the plan is in the next PR to refactor that out into the other 2 libraries. Also it's an extra here I think? But I also like the ability to just specify a prefix

[techalchemy]

the project should just take an abstract idea of an “environment”

I felt this way the entire time I was working on this but I wasn't sure exactly how that should look. Separate class?

[uranusjr]

I feel a separate class would be best, or maybe a dict like what distlib.wheel accepts when it installs the wheel.

[uranusjr]

I think mork lists virtualenv in install_requires, but it is not in extra, so virtualenv will still be unconditionally installed.

[techalchemy]

Refactoring out the virtualenv stuff as is and trying to make an abstraction that handles any environment based on a prefix / set of paths is not really workable so far, everything works except for uninstalling and that is even after I patched uninstallation. Considering I am just going to remove this in the next PR I'm not totally sure it's worth holding the whole thing up over it, I can just combine the two (that is, split out the other two packages along with these changes) if you like

[uranusjr]

Might want a comment explaining why 5.0+ does not work.

[techalchemy]

Not sure it doesn’t but I’ve had a lot of bad experiences leaving this unpinned lately as 5.x is still in alpha and doesn’t tend to play nice with xdist on Travis due to using SQLite as a shared backing store