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

Adds call to os.path.abspath() in pkg_resources.normalize_path() on Cygwin #1335

Merged
merged 3 commits into from Oct 26, 2018

Conversation

Projects
None yet
3 participants
@themiwi
Contributor

themiwi commented Apr 22, 2018

This works around problems that stem from getcwd(3) on Cygwin returning paths containing symlinks. I am not sure at all whether this is a good place to fix it, but that's where I got hit by the issue when doing a python setup.py develop (or pip install -e .).

Adds call to os.path.abspath() in pkg_resources.normalize_path() on C…
…ygwin

This works around problems that stem from getcwd(3) on Cygwin returning paths containing symlinks. I am not sure at all whether this is a good place to fix it, but that's where I got hit by the issue when doing a `python setup.py develop` (or `pip install -e .`).
@pganssle

This comment has been minimized.

Member

pganssle commented Apr 25, 2018

What bug does this fix? I'm not a huge fan of adding platform-specific code that actually works around bugs on those platforms (as opposed to platform-specific code that is designed to deal with actual differences between platforms). It's particularly bad because it's kinda tough to test.

If this is going to be included, I think it needs a test, but I don't really want to add a new executor that runs everything on cygwin. I think something that mocks out sys.platform and some of the other components such that the bug would be triggered if the fix were not in place.

@themiwi

This comment has been minimized.

Contributor

themiwi commented Apr 25, 2018

My problem was with invoking pip install -e . where the working directory path contains a symlink. This fails with a rather obscure message, essentially because the target and egg path are not identical due to the failed symlink resolution.

But I agree, it is an ugly, duct tape type patch. I'll see whether I can find a way to patch Python on Cygwin to behave as expected. The proper fix in Cygwin is probably more problematic since it could break all kinds of things...

@jaraco

This comment has been minimized.

Member

jaraco commented May 15, 2018

Unfortunately, I don't think even having a test case will help because we don't run any tests on cygwin. I also agree with Paul that we really should have a better understanding of the factors that lead to the failure and a clear understanding of why this fix patches it. At least a minimal example posted here would set that context and could be linked to the workaround.

Can you show output from a cygwin shell using only Python that illustrates the conditions that lead to the unexpected condition?

@themiwi

This comment has been minimized.

Contributor

themiwi commented May 15, 2018

Below the transcript of a session that failed:

~ $ mkdir -p ~/tmp/somedir
~ $ cd ~/tmp/somedir
~/tmp/somedir $ mkdir a
~/tmp/somedir $ # make a proper symbolic NTFS directory link, requiring elevated privileges and stuff
~/tmp/somedir $ cygstart --action=runas cmd /c "mklink /d $(cygpath -w $PWD/b) $(cygpath -w $PWD/a)"
~/tmp/somedir $ # check that symlink
~/tmp/somedir $ ls -la
total 16
drwxr-xr-x+ 1 themiwi       Domain Users  0 May 15 20:57 .
drwx------+ 1 themiwi       Domain Users  0 May 15 20:35 ..
drwxr-xr-x+ 1 themiwi       Domain Users  0 May 15 20:46 a
lrwxrwxrwx  1 Administrators Domain Users 38 May 15 20:57 b -> /drives/c/Users/themiwi/tmp/somedir/a
~/tmp/somedir $ cd b/
~/tmp/somedir/b $ git clone https://github.com/pypa/sampleproject.git
Cloning into 'sampleproject'...
remote: Counting objects: 320, done.
remote: Compressing objects: 100% (12/12), done.
remote: Total 320 (delta 4), reused 11 (delta 3), pack-reused 305
Receiving objects: 100% (320/320), 70.48 KiB | 546.00 KiB/s, done.
Resolving deltas: 100% (156/156), done.
~/tmp/somedir/b $ # make a virtual env for cleanliness reasons...
~/tmp/somedir/b $ python3 -m venv ../.venv
~/tmp/somedir/b $ source ../.venv/bin/activate
(.venv) ~/tmp/somedir/b $ cd sampleproject
(.venv) ~/tmp/somedir/b $ # and this will go boom!
(.venv) ~/tmp/somedir/b/sampleproject $ pip install -e .
Obtaining file:///drives/c/Users/themiwi/tmp/somedir/b/sampleproject
Collecting peppercorn (from sampleproject==1.2.0)
  Using cached https://files.pythonhosted.org/packages/45/ec/a62ec317d1324a01567c5221b420742f094f05ee48097e5157d32be3755c/peppercorn-0.5.tar.gz
Installing collected packages: peppercorn, sampleproject
  Running setup.py install for peppercorn ... done
  Running setup.py develop for sampleproject
    Complete output from command /drives/c/Users/themiwi/tmp/somedir/.venv/bin/python3 -c "import setuptools, tokenize;__file__='/drives/c/Users/themiwi/tmp/somedir/b/sampleproject/setup.py';f=getattr(tokenize, 'open', open)(__file__);code=f.read().replace('\r\n', '\n');f.close();exec(compile(code, __file__, 'exec'))" develop --no-deps:
    running develop
    /usr/lib/python3.6/distutils/dist.py:261: UserWarning: Unknown distribution option: 'long_description_content_type'
      warnings.warn(msg)
    /usr/lib/python3.6/distutils/dist.py:261: UserWarning: Unknown distribution option: 'project_urls'
      warnings.warn(msg)
    error: --egg-path must be a relative path from the install directory to /drives/c/Users/themiwi/tmp/somedir/b/sampleproject

    ----------------------------------------
Command "/drives/c/Users/themiwi/tmp/somedir/.venv/bin/python3 -c "import setuptools, tokenize;__file__='/drives/c/Users/themiwi/tmp/somedir/b/sampleproject/setup.py';f=getattr(tokenize, 'open', open)(__file__);code=f.read().replace('\r\n', '\n');f.close();exec(compile(code, __file__, 'exec'))" develop --no-deps" failed with error code 1 in /drives/c/Users/themiwi/tmp/somedir/b/sampleproject/

The base problem seems to be that getcwd(3) does not resolve intermediate symlinks on Cygwin: https://www.cygwin.com/ml/cygwin/2000-10/msg00906.html. My patch just fixes this by calling os.path.abspath() to force this symlink resolution.

But as I already said, this is probably better fixed by patching the python3 package of Cygwin to properly deal with the getcwd(3) antics.

@jaraco jaraco force-pushed the themiwi:patch-1 branch from cf369b0 to c14b6f3 Oct 25, 2018

@jaraco

This comment has been minimized.

Member

jaraco commented Oct 26, 2018

I honestly don't understand why the codecov/patch is failing. It says 75% of diff hit, when actually in a line-by-line comparison, it would have to be hit. But more importantly, I've marked it as pragma: nocover, so it shouldn't be including that function in the coverage stats anyway.

So I'm going to merge this so I can get a release cut.

@jaraco jaraco merged commit d0b88ac into pypa:master Oct 26, 2018

4 of 5 checks passed

codecov/patch 75% of diff hit (target 81.44%)
Details
codecov/project 81.44% remains the same compared to d6d9da8
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@pganssle

This comment has been minimized.

Member

pganssle commented Oct 26, 2018

It may be that we have codecov configured with branch coverage? Or possibly ternary expressions count as multiple lines? Unclear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment