Opened 10 years ago

Closed 9 years ago

Last modified 8 years ago

#13201 closed enhancement (fixed)

patch setuptools to allow for parallel usage

Reported by: R. Andrew Ohana Owned by: Georg S. Weber
Priority: critical Milestone: sage-6.2
Component: build Keywords:
Cc: Jeroen Demeyer, Keshav Kini, Jason Grout Merged in:
Authors: Volker Braun Reviewers: R. Andrew Ohana
Report Upstream: N/A Work issues:
Branch: b276471 (Commits, GitHub, GitLab) Commit:
Dependencies: #11874, #12994 Stopgaps:

Status badges

Description (last modified by Volker Braun)

Currently we have to force the spkgs that use setuptools to build serially because setuptools does not do any file locking with easy_install.pth. This is an inconvenience that has a fairly straightforward fix.

Attachments (2)

easy_install_lock.patch (4.2 KB) - added by R. Andrew Ohana 10 years ago.
new patch applied to setuptools; for review purposes
trac13201.patch (1.5 KB) - added by R. Andrew Ohana 10 years ago.
apply to root repo

Download all attachments as: .zip

Change History (34)

comment:1 Changed 10 years ago by R. Andrew Ohana

Authors: R. Andrew Ohana
Cc: Jeroen Demeyer added
Description: modified (diff)
Status: newneeds_review

comment:2 Changed 10 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

Why re-invent the wheel? Why not use http://docs.python.org/library/fcntl.html#fcntl.lockf?

Also, this should obviously be reported upstream.

comment:3 in reply to:  2 ; Changed 10 years ago by R. Andrew Ohana

Replying to jdemeyer:

Why re-invent the wheel? Why not use http://docs.python.org/library/fcntl.html#fcntl.lockf?

Because I was being dumb and couldn't find that function :).

Also, this should obviously be reported upstream.

Yup, although upstream is basically dead. IMO we should switch to distribute (I'll see about making an SPKG).

Changed 10 years ago by R. Andrew Ohana

Attachment: easy_install_lock.patch added

new patch applied to setuptools; for review purposes

Changed 10 years ago by R. Andrew Ohana

Attachment: trac13201.patch added

apply to root repo

comment:4 Changed 10 years ago by R. Andrew Ohana

Dependencies: #11874#11874, #12994
Status: needs_workneeds_review

comment:5 Changed 10 years ago by R. Andrew Ohana

Milestone: sage-5.3sage-5.4

comment:6 Changed 10 years ago by Jeroen Demeyer

I unfortuntely don't understand setuptools well enough to review this. It seems there are some changes unrelated to the locking. What do they do?

Also, do we really need to do this every time Sage runs (could it be moved to sage-location?):

    # Hack around setuptools since --egg-path isn't fully respected 
    sed -i 's-.*sagenb.*-\.\./\.\./\.\./\.\./devel/sagenb-' \ 
        "$SAGE_LOCAL/lib/python/site-packages/easy-install.pth" 

You also need to handle the case that $SAGE_ROOT isn't writeable.

comment:7 in reply to:  6 Changed 10 years ago by R. Andrew Ohana

Replying to jdemeyer:

I unfortuntely don't understand setuptools well enough to review this. It seems there are some changes unrelated to the locking. What do they do?

Internally setuptools identifies packages based on a normalized path (no absolute paths without symlinks, and a normalized case for case-insensitive filesystems). It reads these from easy-install.pth, and then creates a brand new easy-install.pth if it detects that it needs to. The issue is that it loads the list of packages when it starts, and then writes it back when it finishes, if any changes were made to the file in the meantime, setuptools doesn't detect them, and just overrides them with its new easy-install.pth. What I did was add a bit of code for reloading the file right before writing a new one -- making sure things that were deleted don't pop back up, and making sure things that were added don't disappear.

Also, do we really need to do this every time Sage runs (could it be moved to sage-location?):

    # Hack around setuptools since --egg-path isn't fully respected 
    sed -i 's-.*sagenb.*-\.\./\.\./\.\./\.\./devel/sagenb-' \ 
        "$SAGE_LOCAL/lib/python/site-packages/easy-install.pth" 

You also need to handle the case that $SAGE_ROOT isn't writeable.

Well no you don't have to run it every time sage starts, just every time a package using setuptools is installed, since setuptools always inscribes the absolute path into easy-install.pth (even if you specify a relative path for --egg-path).

comment:8 Changed 10 years ago by Jeroen Demeyer

Since sage-location specifically deals with rewriting paths, I would do it there.

comment:9 Changed 10 years ago by R. Andrew Ohana

I thought that was for rewriting paths when SAGE_ROOT changed. The issue is that setuptools rewrites paths, regardless of what happens to SAGE_ROOT, so we have to overwrite their rewrites. An alternative (now that we have the flask notebook has been merged), is to treat sagenb like any other upstream package, and just install it in site-packages (I don't think the sagenb developers would be opposed to this, we are already planning on doing this in the transition to git).

comment:10 in reply to:  9 ; Changed 10 years ago by Jeroen Demeyer

Replying to ohanar:

I thought that was for rewriting paths when SAGE_ROOT changed.

Well, the reason you need to rewrite the setuptools paths is to allow relocation, right? (or am I missing something?)

comment:11 in reply to:  10 Changed 10 years ago by R. Andrew Ohana

Cc: Keshav Kini Jason Grout added

Replying to jdemeyer:

Well, the reason you need to rewrite the setuptools paths is to allow relocation, right? (or am I missing something?)

Well we also support clones for sagenb, which will brake every time setuptools is run (it readlinks everything). If we don't care about that, then we could.

I'm CCing Keshav and Jason on this ticket, they might have an opinion on how we handle this (since currently it only affects sagenb).

comment:12 Changed 10 years ago by Jason Grout

Since sagenb is now distributed without a repository, it might make sense for it not to be installed in SAGE_ROOT/devel/. The reason for it to be in SAGE_ROOT/devel/sagenb is so that the barrier is way lower for people wanting to patch it and work with it. If we're making them clone the git repository anyway, the barrier isn't lower.

So what do you think, kini? Should we just install it like a normal spkg? If they want to develop, they need to git clone and do sage setup.py develop?

(personally, I'm mildly in favor of going back to installing the git repo in SAGE_ROOT/devel/sagenb...)

Last edited 10 years ago by Jason Grout (previous) (diff)

comment:13 Changed 10 years ago by Keshav Kini

Absolutely. I 100% think that sagenb should be installed like any other python package, i.e. in the site packages directory.

comment:14 Changed 10 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

In any case, this needs work because it assumes that $SAGE_ROOT is writable and it needs to be rebased.

comment:15 in reply to:  3 Changed 10 years ago by François Bissey

Replying to ohanar:

Replying to jdemeyer:

Why re-invent the wheel? Why not use http://docs.python.org/library/fcntl.html#fcntl.lockf?

Because I was being dumb and couldn't find that function :).

Also, this should obviously be reported upstream.

Yup, although upstream is basically dead. IMO we should switch to distribute (I'll see about making an SPKG).

At this point I'd like to point you to SPKG.txt and actually read it:

= setuptools =

== Description ==

setuptools is a collection of enhancements to the Python distutils (for Python 2.3.5 and up on mo
st platforms; 64-bit platforms require a minimum of Python 2.4) that allow you to more easily bui
ld and distribute Python packages, especially ones that have dependencies on other packages.

Website: http://pypi.python.org/pypi/setuptools/

The present spkg is based on the fork of setuptools known as distribute.
Website: http://pypi.python.org/pypi/distribute

And

=== setuptools-0.6.16 (Francois Bissey, June 1, 2011) ===
 * Switch to the "distribute" fork of setuptools and update to 0.6.16, 
 * adopt a Gentoo patch to avoid warnings with python-2.7 (works with 2.6 too). 
 * Remove the two windows binaries and patch so setup.py doesn't
   try to install them.

This was a pre-requisite to the upgrade to python-2.7.3. Fell free to upgrade to a newer version of distribute and change the name if you want too.

comment:16 Changed 10 years ago by R. Andrew Ohana

Yeah, I made that remark about 5 minutes before completely reading through the SPKG.txt. I haven't really looked at this in awhile, but I probably won't be touching this soon, as I'm really trying to put all of my time into working on the transition to git.

comment:17 Changed 10 years ago by François Bissey

I checked if you said that in the ticket but didn't see it so I assumed... too much. ;)

Last edited 10 years ago by François Bissey (previous) (diff)

comment:18 Changed 9 years ago by Jeroen Demeyer

Milestone: sage-5.11sage-5.12

comment:19 Changed 9 years ago by For batch modifications

Milestone: sage-6.1sage-6.2

comment:20 Changed 9 years ago by Volker Braun

Branch: u/vbraun/parallel_setuptools

comment:21 Changed 9 years ago by git

Commit: a899c2ccc2ea5dc2fa8c3194274194a09f670a47

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

a899c2cremove fake deps

comment:22 Changed 9 years ago by git

Commit: a899c2ccc2ea5dc2fa8c3194274194a09f670a4739481e1bbc144264354e196c23a839992305bef9

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

39481e1avoid duplicated entries in pth file

comment:23 Changed 9 years ago by Volker Braun

Authors: R. Andrew OhanaVolker Braun, R. Andrew Ohana
Description: modified (diff)

New attempt... The patch didn't apply, so I wrote a different version. It is now IMHO clearer though perhaps at the cost of rewriting a pth file even if it is not dirty. But then it is hardly a performance-critical step.

comment:24 Changed 9 years ago by Volker Braun

Status: needs_workneeds_review

We can't push the changes upstream since fcntl is unix only but setuptools has to work on (non-cygwin) windows

comment:25 Changed 9 years ago by Volker Braun

Priority: majorcritical

Critical as we currently have some races as I noticed in parallel testing.

comment:26 Changed 9 years ago by R. Andrew Ohana

Authors: Volker Braun, R. Andrew OhanaVolker Braun

Looks fine, although I'll have to test it out. It would be good to get some sort of proper locking in upstream -- maybe doing something along the lines of http://code.activestate.com/recipes/65203/.

comment:27 Changed 9 years ago by R. Andrew Ohana

Reviewers: R. Andrew Ohana

The patch version of setuptools will need a version bump to force a rebuild. Also, it would be good to have the patch documented in the SPKG.txt. Pending those two things, positive review.

comment:28 Changed 9 years ago by git

Commit: 39481e1bbc144264354e196c23a839992305bef925f77eee5aa1c2c645717523829b6ce9bad2a688

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

25f77eeadded documentation, patchlevel bump

comment:29 Changed 9 years ago by Volker Braun

Status: needs_reviewpositive_review

Done.

comment:30 Changed 9 years ago by git

Commit: 25f77eee5aa1c2c645717523829b6ce9bad2a688b2764713f797c81d157ce681f878ce7a52d70e44
Status: positive_reviewneeds_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

b276471resolved merge conflict

comment:31 Changed 9 years ago by Volker Braun

Branch: u/vbraun/parallel_setuptoolsb2764713f797c81d157ce681f878ce7a52d70e44
Resolution: fixed
Status: needs_reviewclosed

comment:32 Changed 8 years ago by Jeroen Demeyer

Commit: b2764713f797c81d157ce681f878ce7a52d70e44

You just broke sagenb: #17268

Note: See TracTickets for help on using tickets.