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.

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

Fix --target not working with --editable #9636

Merged
merged 4 commits into from
Feb 28, 2021
Merged

Fix --target not working with --editable #9636

merged 4 commits into from
Feb 28, 2021

Conversation

dwt
Copy link
Contributor

@dwt dwt commented Feb 20, 2021

as setup.py develop does not understand --home but instead requires the --install-dir option.

Partially fixes #4390

… understand --home but instead requires the --install-dir option.

Parially fixes pypa#4390
@dwt dwt changed the title Fix --target not working with --editable Fix --target not working with --editable Feb 20, 2021
@dwt dwt changed the title Fix --target not working with --editable Fix --target not working with --editable Feb 20, 2021
@dwt
Copy link
Contributor Author

dwt commented Feb 20, 2021

This would also fix pypa/setuptools#392

news/4390.bugfix.rst Outdated Show resolved Hide resolved
dwt and others added 2 commits February 20, 2021 20:19
Thanks @webknjaz

Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM based on a quick skim, but I'm not digging into the setuptools code here.

If someone more familiar w/ setuptools' code (maybe @jaraco or @pganssle?) think this is OK, I'm happy to merge.

@jaraco
Copy link
Member

jaraco commented Feb 21, 2021

At first blush, it's not working for me:

~ $ pip-run git+https://github.com/dwt/pip -q -- -m pip-run -e ~/m/jaraco.functools -- -c "import jaraco.functools"
Obtaining file:///Users/jaraco/m/jaraco.functools
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
    Preparing wheel metadata ... done
Collecting more-itertools
  Using cached more_itertools-8.7.0-py3-none-any.whl (48 kB)
Installing collected packages: more-itertools, jaraco.functools
  Running setup.py develop for jaraco.functools
Successfully installed jaraco.functools more-itertools-8.7.0
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ModuleNotFoundError: No module named 'jaraco'

pip-run uses --target under the hood to create a temporary target into which to install and then adds that to sys.path. In this case, I use pip-run to install the proposed version of pip, then use it to invoke pip-run again to install some local package in editable mode. If it worked, the package would be available in the third interpreter, but it's not. I get a similar error for a package that doesn't use a namespace package (tempora).

I tried without pip-run (for the target install), but it still doesn't produce a usable install:

draft $ mkdir pip9636
draft $ pip-run git+https://github.com/dwt/pip -q -- -m pip install -t pip9636 -e ~/m/tempora
Obtaining file:///Users/jaraco/m/tempora
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
    Preparing wheel metadata ... done
Collecting pytz
  Using cached pytz-2021.1-py2.py3-none-any.whl (510 kB)
Collecting jaraco.functools>=1.20
  Using cached jaraco.functools-3.2.1-py3-none-any.whl (6.7 kB)
Collecting more-itertools
  Using cached more_itertools-8.7.0-py3-none-any.whl (48 kB)
Installing collected packages: more-itertools, pytz, jaraco.functools, tempora
  Running setup.py develop for tempora
Successfully installed jaraco.functools-3.2.1 more-itertools-8.7.0 pytz-2021.1 tempora
draft $ env PYTHONPATH=pip9636 python
Python 3.9.0 (v3.9.0:9cf6752276, Oct  5 2020, 11:29:23) 
[Clang 6.0 (clang-600.0.57)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import tempora
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'tempora'
>>> ^D
draft $ tree -L 1 pip9636
pip9636
├── calc-prorate
├── jaraco
├── jaraco.functools-3.2.1.dist-info
├── more_itertools
├── more_itertools-8.7.0.dist-info
├── pytz
├── pytz-2021.1.dist-info
└── tempora.egg-link

6 directories, 2 files

It seems there's no .pth file in the target, so I guess it's no surprise that the develop-installed package doesn't appear to be present. There is an egg-link, but as far as I know, Python doesn't recognize that at run time.

This patch does correct the "error: option --home not recognized" but it still doesn't provide a viable install. Making that work may require changes to Setuptools. I haven't yet looked into what behaviors affect making an editable package visibly installed.

@jaraco
Copy link
Member

jaraco commented Feb 21, 2021

Aha. So the reason setup.py develop works in the normal case is because it adds the editable files to $site_packages/easy-install.pth, and that's what makes the code present at run time. Perhaps there's a patch that would serve this use-case in Setuptools, or it may be the case that more broad solution for editable installs needs to be developed (designed, implemented).

This patch seems fine, but I'd be curious - does this patch provide any benefit without improved --install-dir support in Setuptools?

@dwt
Copy link
Contributor Author

dwt commented Feb 22, 2021

Aha. So the reason setup.py develop works in the normal case is because it adds the editable files to $site_packages/easy-install.pth, and that's what makes the code present at run time. Perhaps there's a patch that would serve this use-case in Setuptools, or it may be the case that more broad solution for editable installs needs to be developed (designed, implemented).

This patch seems fine, but I'd be curious - does this patch provide any benefit without improved --install-dir support in Setuptools?

I do not claim to fully understand the internals of setuptools, but it is my understanding that --target only installs the package to the folder in question. You still need to add the directory / the packages inside it to the install path to get python to be able to import them. The project that prompted me to do the patch uses code like this to achieve that:

def add_site(path):
    """This adds a path to as proper site packages to all associated import
    systems.  Primarily it invokes `site.addsitedir` and also configures
    pkg_resources' metadata accordingly.
    """
    site.addsitedir(path)
    ws = pkg_resources.working_set
    ws.entry_keys.setdefault(path, [])
    ws.entries.append(path)
    for dist in pkg_resources.find_distributions(path, False):
        ws.add(dist, path, insert=True)

(Note to self: The code above doesn't seem quite right, as it seems to add the path twice to ws.entries).

@dwt
Copy link
Contributor Author

dwt commented Feb 24, 2021

This patch seems fine, but I'd be curious - does this patch provide any benefit without improved --install-dir support in Setuptools?

I think it serves as the baseline to make the install work, and then build upon it to make discovery of installed packages simpler in the relevant packages (not sure yet what package that should be.)

@uranusjr
Copy link
Member

IMO we should

  1. Open an issue on pypa/setuptools and ask if the maintainers intend to get this fixed eventually (if not, there's no point merging this)
  2. In the meantime, add a check to explicitly fail when --target is specified with --editable (with a message linking to the setuptools issue) to avoid user confusion.
  3. If setuptools is willing to fix the issue, merge this.
  4. Remove the explicit check when setuptools merges a fix.

@jaraco
Copy link
Member

jaraco commented Feb 24, 2021

You still need to add the directory / the packages inside it to the install path to get python to be able to import them.

pip-run does something like what you describe, but it's still insufficient because there's nothing that setuptools installs into the target that Python recognizes as a link to the package. The egg-link it installs is ignored.

IMO we should

  1. Open an issue on pypa/setuptools and ask if the maintainers intend to get this fixed eventually (if not, there's no point merging this)

In the abstract, Setuptools is interested in solving this issue. My preference would be to solve the 'editable' problem in the packaging space and not just patch the Setuptools implementation, though I'd also accept a patch to Setuptools to address this specific weakness.

@uranusjr
Copy link
Member

uranusjr commented Feb 24, 2021

My preference would be to solve the 'editable' problem in the packaging space and not just patch the Setuptools implementation

Me too, but the interest around this is just insufficient to move this forward. A part of me want to just break editables entirely to force people’s hands into making something happen.

In the same vein, I don’t have any interest in merging this until someone to actually work on a setuptools patch. (Note that this is for me personally; other pip maintainers may feel differently, and I would feel most happy if this can fix itself without me spending any effort.)

@dwt
Copy link
Contributor Author

dwt commented Feb 25, 2021

Well then, what are the possible steps to get this fixed? In Setuptools? In the package space?

For Setuptools I would imagine that it needs to recognise .egg-link files and activate those packages if the directory is on the python path. Or is it something else that should happen?

@piotr-dobrogost
Copy link

piotr-dobrogost commented Feb 25, 2021

it needs to recognise .egg-link files

I think these files are relic of ancient times when people used easy_install to install Python packages and are not and should not be supported by any modern tool.

Aha. So the reason setup.py develop works in the normal case is because it adds the editable files to $site_packages/easy-install.pth

.pth files are established way of extending PYTHONPATH and I think the right way to go here. Either just add editables to this file or come up with some different .pth file for this purpose. Or maybe it should be a separate .pth file per editable package installed?

I wonder if issue #9471 won't have similar problem?

@dwt
Copy link
Contributor Author

dwt commented Feb 25, 2021

.pth files are established way of extending PYTHONPATH and I think the right way to go here. Either just add editables to this file or come up with some different .pth file for this purpose. Or maybe it should be a separate .pth file per editable package installed?

I may be wrong here, but my understanding of .pth files is that they basically add directories to the python path, while .egg-link files basically act as symlinks to link a package in a folder which (hopefully) is already part of the python path. Also, I think handling of metadata is slightly special, because it is taken from within the project folder, instead of being installed alongside the package in the site directory.

Since Symlinks seem to be supported even in Windows nowadays Maybe just using an actual symlink to the project is the right way to go?

To support working on editable installed packages it's metadata should really be in the project instead of the site folder, so either that needs / should be symlinked too, or should be taken from inside the project initially?

I'd love some thoughts on these points.

@piotr-dobrogost
Copy link

@pfmoore
Copy link
Member

pfmoore commented Feb 25, 2021

There's plenty of context here. We have a proof of concept implementation in the form of my editables package, plus an initial implementation PR in pip (which I can't recall the link for, and don't know the status of - it's in that Discourse thread, though).

Every time we discuss standardising editables, though, we get a certain way and then everyone loses motivation/focus and things stall again, as @uranusjr noted.

I don't know if this change wants to get sucked into the whole "standardise editables" debate, though. But if @jaraco feels it's better to do that than some sort of tactical hack to setuptools, I can understand that.

@pfmoore
Copy link
Member

pfmoore commented Feb 25, 2021

BTW, my personal opinion is similar to @uranusjr - I feel that we shouldn't keep trying to prop up the existing editable mechanism. If it doesn't work, the solution is to sort out standardising a replacement. If the community isn't motivated to do that, fine, but in that case we should put up with what we have, rough edges and breakages and all.

@dwt
Copy link
Contributor Author

dwt commented Feb 26, 2021

I am not sure why feel that lately the sentiment always seems to be that all or nothing solutions are required to accept any patch. What happened to small incremental improvements?

This patch doesn't break anything that wasn't broken before, enables a use-case that wasn't available before, and allows further incremental improvements in other places, all without blocking a standardisation attempt to unify / rework editable installs.

I would like to work on a patch to setuptools next that makes it easier to work with *.egg-link files. Probably by first documenting how to enable them if installed with --install-dir=. All of that hinges on pip not actively breaking my workflow though - hence this patch.

@jaraco
Copy link
Member

jaraco commented Feb 26, 2021

I'm committed to addressing this in Setuptools, just not on any particular timeline. The argument from dwt is compelling and I recommend to accept this change to facilitate a tactical fix in Setuptools.

@uranusjr uranusjr merged commit 550270c into pypa:master Feb 28, 2021
davidlatwe added a commit to davidlatwe/rezup that referenced this pull request Apr 21, 2021
pip flag `-e` and `--target` cannot be used at
the same time, they are trying to resolve this
but could take a while. See pypa/pip#9636.

So I decide not to install rez with `--target`
for all venv to access, just re-install it in
every venv.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The --target option clashes with other command line flags and config files
7 participants