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

Actually remove DESTDIR staging for Python packages to eliminate race conditions during Python package installations #32361

Closed
mkoeppe opened this issue Aug 10, 2021 · 28 comments

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Aug 10, 2021

Follow-up from #29585, which forgot to include a crucial commit.

Race conditions are still present in parallel builds:
https://github.com/sagemath/sage/runs/3274951485?check_suite_focus=true

  [cython-0.29.21]   user	1m11.995s
  [cython-0.29.21]   sys	0m2.867s
  [cython-0.29.21]   Copying package files from temporary location /sage/local/var/tmp/sage/build/cython-0.29.21/inst to /sage/local
  [cython-0.29.21]   cp: cannot create directory '/sage/local/./lib/python3.9/site-packages/Cython/__pycache__': File exists
  [cython-0.29.21]   ************************************************************************
  [cython-0.29.21]   Error copying files for cython-0.29.21.
  [cython-0.29.21]   ************************************************************************

CC: @vbraun @jhpalmieri @dimpase

Component: build

Author: Matthias Koeppe

Branch/Commit: 378a034

Reviewer: John Palmieri

Issue created by migration from https://trac.sagemath.org/ticket/32361

@mkoeppe mkoeppe added this to the sage-9.4 milestone Aug 10, 2021
@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 10, 2021

Author: Matthias Koeppe

@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 10, 2021

@dimpase
Copy link
Member

dimpase commented Aug 10, 2021

comment:3

the branch field was removed?

I am away from kbds till Sat, so I can review only visually

@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 10, 2021

comment:4

... editing mistake


New commits:

8fccf3ebuild/bin/sage-dist-helpers (sdh_store_and_pip_install_wheel): No more DESTDIR staging for Python packages (unless SAGE_SUDO is set)

@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 10, 2021

Commit: 8fccf3e

@jhpalmieri
Copy link
Member

comment:5

I'd never set SAGE_SUDO="sudo -E" before, but now I'm trying it and pip is failing to build. In brief experiments, it seems to succeed without this branch.

@jhpalmieri
Copy link
Member

Attachment: pip-21.1.2.log

@jhpalmieri
Copy link
Member

comment:6

Just to confirm: if I do sudo make distclean; ./configure; make toolchain; make pip, it fails with this branch, succeeds with 9.4.rc2 (assuming that SAGE_SUDO is set).

@mkoeppe
Copy link
Member Author

mkoeppe commented Aug 22, 2021

comment:7

sudo make distclean is a scary operation, perhaps you just wanted sudo rm -rf local/?

As make distclean can run ./config.status --recheck, this may create root-owned files in the source directory.

@mkoeppe mkoeppe modified the milestones: sage-9.4, sage-9.5 Aug 22, 2021
@jhpalmieri
Copy link
Member

comment:9

Replying to @mkoeppe:

sudo make distclean is a scary operation, perhaps you just wanted sudo rm -rf local/?

As make distclean can run ./config.status --recheck, this may create root-owned files in the source directory.

It is still the case that after doing export SAGE_SUDO="sudo -E", then pip fails to build.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 6, 2021

Changed commit from 8fccf3e to c7a1171

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 6, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

c7a1171build/bin/sage-dist-helpers (sdh_store_and_pip_install_wheel): Do not use SAGE_SUDO for pip install

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 6, 2021

comment:11

Found the mistake, fixed

@jhpalmieri
Copy link
Member

comment:12

That helps, thanks. In the log for sage_docbuild, I see messages like this, and they seem to be new with this branch:

Value for scheme.platlib does not match. Please report this to <https://github.com/pypa/pip/issues/9617>

(This is again with SAGE_SUDO set.)

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 6, 2021

comment:13

Replying to @jhpalmieri:

In the log for sage_docbuild, I see messages like this, and they seem to be new with this branch:

Value for scheme.platlib does not match. Please report this to <https://github.com/pypa/pip/issues/9617>

Thanks, I see them too. I'll investigate

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 6, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

a05d57eMerge tag '9.5.beta0' into t/32361/actually_remove_destdir_staging_for_python_packages_to_eliminate_race_conditions_during_python_package_installations

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 6, 2021

Changed commit from c7a1171 to a05d57e

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 6, 2021

comment:15

Unchanged with 9.5.beta0 merged (which has #32046)

Also upgrading pip to latest (21.2.4) does not make a difference

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 6, 2021

comment:16

Newer pip issue: pypa/pip#10151

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 6, 2021

Branch pushed to git repo; I updated commit sha1. New commits:

378a034build/bin/sage-dist-helpers (sdh_store_and_pip_install_wheel): Fix for script packages with SAGE_SUDO set

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 6, 2021

Changed commit from a05d57e to 378a034

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 6, 2021

comment:18

OK, found a solution. Not a pip bug.

@jhpalmieri
Copy link
Member

Reviewer: John Palmieri

@jhpalmieri
Copy link
Member

comment:19

Okay, I think it's ready to go. I don't know what else to do to stress-test it.

@mkoeppe
Copy link
Member Author

mkoeppe commented Sep 7, 2021

comment:20

Thank you!

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

No branches or pull requests

4 participants