Implementation of stored pipx metadata

[itsayellow]

This is in reference to Issue #220

I've implemented a pipxrc file in each venv directory that contains the following:

• package_or_url - keeping track of the origin of the venv package
• venv_metadata - keeping track of venv package metadata so uninstall is possible even if the python pointer in the venv bin/ directory is no longer valid
• injected_packages - keeping track of all injected packages, allowing reinstall to re-inject them all

I realize that I don't have this in a proper named branch, and the commit log is a bit messy and winding (I spent time figuring out the best way to implement things.) I'm happy to fix all of the above, but I thought it best to submit the code I have to allow for discussion.

Having a pipxrc really helps a lot! I modified install, uninstall, upgrade, reinstall-all, upgrade-all: all of these commands behave much better, especially with URL-based packages. In this code I also modified reinstall-all to re-inject all injected packages, which fixes Issue #63.

I also tested reinstall-all with broken symlinks to python inside of the venv, and it works beautifully. This would allow this pull request to fix Issue #146 by allowing reinstall-all to update all the venvs after a system python upgrade. Note: I would recommend that reinstall-all use DEFAULT_PYTHON as the default for the python argument instead of requiring that argument.

Some outstanding issues:

• I uncovered a bug by looking at the code (uninstall does not uninstall injected apps binaries #221). I believe pipxrc can help with this, because it remembers both the injected packages and their inject flags. But I have not addressed this Issue with this pull request
• I believe that upgrade can drop the '--spec' option now that each venv remembers its own package_or_url, but I didn't code this.
• I did not handle an exception if pipx is unable to write pipxrc. I was unsure if this was necessary
• I am not sure exactly whether _run_post_install_actions() affects the venv metadata. If it does, then pipxrc venv_metadata should be updated afterwards. (This is not done in my current pull request)

[cs01]

Nice, thanks for taking the time to do this! I have a pretty busy schedule so unfortunately I can't give this a full review right now. I will take a look when I get a chance, probably within the next couple of weeks.

I am actually the only person who maintains/reviews PRs for this project so unfortunately no one else can step in and "officially" review this. But if you are reading this and are interested in this feature, please feel free to install this branch of pipx, see how it works for you, and leave any feedback here.

[itsayellow]

I now have these commits cherry-picked/copied to a new branch 'pipxrc_implement' in my fork, so let me know if it would be better to replace this pull request with a new one based on that non-master branch.

[itsayellow]

I moved writing pipxrc to after _run_post_install_actions() in case this affects the venv metadata.

I also add pipx install args to what is stored in pipxrc: pip_args, venv_args, include_dependencies

I think reinstall-all() and upgrade-all() should not use global arguments for pip_args, venv_args, include_dependencies, but instead use the pipxrc stored versions.

[itsayellow]

I also noticed something I'm not sure how to address: If install uses a local path as a spec, pipxrc should really record the absolute path version of this path in order to work properly in the future.

I wasn't sure if there was a sure-fire way to parse the spec to know if what we were passed was a path or a URL or some other type of spec. I'm wondering if there's a good way to get this information from pip or the venv, so we know for sure whether pip thought it was being passed a local path.

[itsayellow]

Changed reinstall-all() and upgrade-all() so they do not use global arguments for pip_args, venv_args, include_dependencies, but instead use the pipxrc stored versions for each different package/venv.

[clbarnes]

I wasn't sure if there was a sure-fire way to parse the spec to know if what we were passed was a path or a URL or some other type of spec

The way pipx does this currently is if urllib.parse.urlparse(args.spec).scheme. We could always check that the scheme is not file:// as well. For checking whether it's a (valid) local path you could just do Path(fpath).is_file().

One other thing to consider is the utility of a pipxrc-like file as a way to export all of a user's pipx-installed packages, and then import them from that file on a clean machine. It would be great to share machinery in that regard - I implemented a requirements.txt -like export in #224 but without injections and so on its utility is limited for this kind of config.

If we were to consider the possibility of the pipxrc, or a superset of it, becoming a human-readable/writeable config file for multiple pipx packages, would you still pick JSON? Plenty of apps do, and it's nicely in stdlib and nest-able, but for human interaction something like TOML is pretty nice so long as it's not too deeply nested. Either way I don't see any reason not to have an informative file extension.

[itsayellow]

I wasn't sure if there was a sure-fire way to parse the spec to know if what we were passed was a path or a URL or some other type of spec

The way pipx does this currently is if urllib.parse.urlparse(args.spec).scheme. We could always check that the scheme is not file:// as well. For checking whether it's a (valid) local path you could just do Path(fpath).is_file().

Thanks! I couldn't find this, it helps a lot.

One other thing to consider is the utility of a pipxrc-like file as a way to export all of a user's pipx-installed packages, and then import them from that file on a clean machine. It would be great to share machinery in that regard - I implemented a requirements.txt -like export in #224 but without injections and so on its utility is limited for this kind of config.

If we were to consider the possibility of the pipxrc, or a superset of it, becoming a human-readable/writeable config file for multiple pipx packages, would you still pick JSON? Plenty of apps do, and it's nicely in stdlib and nest-able, but for human interaction something like TOML is pretty nice so long as it's not too deeply nested. Either way I don't see any reason not to have an informative file extension.

I think right now the implementation of pipxrc currently has all the information needed to "freeze" all pipx packages. (To properly reinstall-all is basically the same thing, just not exported to a file.). I like json for a machine-readable format just because it's so standard and clean, and because @cs01 suggested it when suggesting pipxrc.

My basic approach has been that pipxrc in each venv is something that it would be nice for the user not to be monkeying with, so that we don't have to get creative with parsing it (i.e. accepting quirks, etc.) and it would be kept clean and valid.

That being said, I think it would be pretty trivial to visit each venv, read in the pipxrc, and output to any kind of "frozen" file format that is more human-readable. Personally I use YAML a lot, but I don't have a strong opinion on the best one. I tend to think that a "freeze" function, while using pipxrc as a source of information, can be a separate topic make different choices for file format, etc.

[clbarnes]

I think right now the implementation of pipxrc currently has all the information needed to "freeze" all pipx packages.

Awesome! That's what I was hoping for.

Personally I use YAML a lot, but I don't have a strong opinion on the best one.
I tend to think that a "freeze" function, while using pipxrc as a source of information, can be a separate topic make different choices for file format, etc.

Agreed! Happy for this to be punted that out of scope for now. An analogue would be pipenv, which uses TOML for its human-editable configuration file, and JSON for its machine-only lock file.

The comparisons have been done to death, most notably for our case in the pyproject.toml. TOML is much more machine-readable than YAML, much more standard than INI, and much more human-editable than JSON. It would be my choice so long as we don't have to do deep nesting, or use references or whatever.

[itsayellow]

I put a pipxrc_info_template dict inside of pipxrc.py, as well as adding a pipxrc_version number inside of it. I'm hoping that pipxrc_version can be used to aid in future-proofing pipxrc, so incompatible changes can bump this version number if necessary.

This latest set of changes is an attempt to contain most code affecting pipxrc file format to pipxrc.py. But in reality, what may be necessary to do this properly is a Pipxrc class to handle all reading/writing to pipxrc, in order to make sure only pipxrc.py affects the pipxrc file format.

[itsayellow]

I converted pipxrc to a class-based interface around class Pipxrc. I like how clean it is, and it also keeps any code that might change the format of pipxrc files in pipxrc.py.

I do need to ask for some help. I can't figure out how to eliminate the following mypy error:
pipx/pipxrc.py:119: error: Unsupported target for indexed assignment

It comes for the line:
self.pipxrc_info["injected_packages"][package] = {....}

I originally initialize self.pipxrc_info["injected_packages"] = None, to indicate an invalid state. I then change it to {}, which mypy seems ok with, but then nesting a dict value inside this dict is not ok by mypy.

Can anyone help me understand the proper way to do this typing correctly?

[itsayellow]

I may just have to break apart the giant heterogenously-typed dict into separate class members, and then assemble them only upon serialization to accommodate mypy...?

[clbarnes]

I think in some modes it's quite easy for mypy to get upset about Any annotations, but I haven't used it that much.

I also like the class representation! And I agree with splitting the dict out so that the class truly represents the information in the pipxrc file, rather than wrapping a dict which does. The class could be a dataclass or NamedTuple. Then it could have a to_dict(self) method, and a from_dict(cls, d) class method. Then the setters and getters may not be necessary in that scheme, too. The sub-dict could be a separate class if that made sense.

I might err on the side of the default constructor using raw passed in values, and then having a from_dir(cls, venv) class method which finds the directory, reads the file, passes the information to from_dict. Would be a bit more flexible and explicit.

[itsayellow]

Ok, thanks for the advice. I did try NamedTuple but the main problem with that is...it’s a Tuple and hence immutable. Frustrating that there’s no NamedList.

…

On Fri, Sep 27, 2019 at 8:07 AM Chris Barnes ***@***.***> wrote: I think in some modes it's quite easy for mypy to get upset about Any annotations, but I haven't used it that much. I also like the class representation! And I agree with splitting the dict out so that the class truly represents the information in the pipxrc file, rather than wrapping a dict which does. The class could be a dataclass or NamedTuple. Then it could have a to_dict(self) method, and a from_dict(cls, d) class method. Then the setters and getters may not be necessary in that scheme, too. The sub-dict could be a separate class if that made sense. I might err on the side of the default constructor using raw passed in values, and then having a from_dir(cls, venv) class method which finds the directory, reads the file, passes the information to from_dict. Would be a bit more flexible and explicit. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#222>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC732GHK2U37R3ROL5HIVCTQLYOTNANCNFSM4IY4ZBBQ> .

[clbarnes]

Ah yes, forgot we were mutating it. Dataclass should work but doesn't add much convenience.

[itsayellow]

I'm still a little stuck on how to determine whether the package spec is definitively a local directory or a pypi package.

If the --spec has no url-scheme, and has a slash in it, then this is a non-valid pypi name and we can be sure it's a local directory.

However, if the --spec is just one word, it could be either a local directory or a pypi package.

I can't find a way of querying pip as to the source it found for a package. pip list only seems to list where a package is installed: if the editable -e option is set then this does point to the local source directory, but if no editable -e option was used then the package appears just as any other pypi package even if it was sourced from a local directory.

The only thing I can think of is to do a pip search for any one-word package --spec we receive, to know whether or not it exists on pypi. This seems like it might take a little while and be unnecessarily broad (searching for related names as well.) But as of now I can't think of another way of doing it.

[itsayellow]

I did find a quick way to tell if a package is local, but I can't decide if it's a bit too hacky.

It involves using an empty requirement with install, which can never be satisfied. The resulting error message either indicates the versions that do exist for the pypi package, or that the package has no versions and thus is not on pypi.

compy:~$ pip install doesntexist==
Collecting doesntexist==
  ERROR: Could not find a version that satisfies the requirement doesntexist== (from versions: none)
ERROR: No matching distribution found for doesntexist==
compy:~$ pip install pycowsay==
Collecting pycowsay==
  ERROR: Could not find a version that satisfies the requirement pycowsay== (from versions: 0.0.0.1)
ERROR: No matching distribution found for pycowsay==

This is quick but I worry it's a bit hacky and fragile.

[itsayellow]

_abs_path_if_local() is a little messy, but I believe it works to return an absolute path if the package_or_url is a local path.

I tried to only use pip search as a last resort, first checking if the path actually exists, and then seeing if the package_or_url is even a valid pypi package name.

[itsayellow]

I think I'm going to stop the commits and call this code ok by me.

@cs01 or anyone can feel free to let me know if there's something that needs fixing.

[cs01]

I am working on improvements to unit tests and metrics on coverage. After that lands, we can pull those changes into your branch and make sure there are adequate tests for the new functionality. I'll give it a review at that time. I haven't forgotten about this 😄 . Thanks for putting this together!

[cs01]

I've been pretty busy, but have slowly been making progress on the unit tests. They are looking much better. I'll tag you when I create the PR. Might be another week or two still.

[clbarnes]

Any chance the pipxrc could have a .json extension? It makes it more readable for syntax-aware editors/ viewers and is more explicit in general.

[itsayellow]

Totally no problem. I don't even have a problem if the stem is called something other than "pipxrc". I suppose it could be something like pipx.json or pipx-info.json.

[cs01]

I updated pipx's unit tests and some of pipx's code, but now there is a merge conflict in this PR. But it should be more straightforward to write tests now and measure coverage.

[cs01]

Do you mind if I pull in the changes and fix merge conflicts? I will do that and give the new rc file a try and then give a proper review.

[clbarnes]

Anything holding this back? Thanks to @itsayellow and everyone else involved for the herculean effort in this PR, and keeping it up to date as master has been changing under your feet; really looking forward to seeing it in action!

[cs01]

Definitely huge 👏 for @itsayellow. It was a lot of work over a long period of time. I have to give it another review still. I'll try to get to it soon.

[itsayellow]

Definitely huge 👏 for @itsayellow. It was a lot of work over a long period of time. I have to give it another review still. I'll try to get to it soon.

Thanks, I'm eager to see it merged too! I think there were only 2-3 remaining questions I had for @cs01 of what is not "resolved" from the last review.

[cs01]

I will be out of town until Monday, so I can't review until next week. If anyone else has any feedback or would like to review, please jump in!

[cs01]

I just looked over the remaining comments and resolved them. With that, I will merge this pull request 🎉 . Thanks again to everyone involved, especially @itsayellow! This is going to be a huge usability improvement. I will let you do the honors of merging it.

After it's merged, the changelog should be updated with any changes from the last release, and then I can make a beta release for people to try this out.

[itsayellow]

Thanks so much to all the reviewers. 👍