Replace pip2/3-lock with a generic sage-flock command

[embray]

I suggested something like this on #21672 but didn't have immediate need to follow up on it.

Something like this will be useful for making Windows builds more reliable, however, as there are some operations that can fail if they are run simultaneously during parallel builds.

In particular I'm trying to trace down failures with DLL rebasing, and at least part of the problem is that rebase can fail if it's being multiple times simultaneously as can happen during a parallel build (especially toward the end of the build when lots of Python packages are being installed--these are fast and often results in multiple rebases being run simultaneously).

With tickets like #22509 it will also be possible to make further restrictions, such as not running rebase while a package is being uninstalled.

CC: @jdemeyer

Component: build

Author: Erik Bray

Branch/Commit: b6e4a62

Reviewer: Jeroen Demeyer

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

[embray]

Author: Erik Bray

[embray]

Commit: 63f4b2d

[embray]

Branch: u/embray/build/lockfile

[embray]

comment:1

(CC: jdemeyer, original author of the script)

---

New commits:

63f4b2d

Adds a new sage-flock command based on the original pip2-lock command, but generalized to

[embray]

comment:2

Apparently this has a bug...

[sagetrac-git]

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

5e89b68

argparse.FileType can be handy, but it doesn't create directories.

[sagetrac-git]

Changed commit from 63f4b2d to 5e89b68

[embray]

comment:5

ping

I actually increasingly need this as it's difficult to get through a parallel build on Cygwin now without it failing a few times due to the lack of locking around rebase.

[jdemeyer]

comment:6

Why did you remove sys.stderr.flush()?

[jdemeyer]

Reviewer: Jeroen Demeyer

[jdemeyer]

comment:7

I think it would be good to mention somewhere in a comment that this may be used in build scripts, but only for packages depending on Python.

The help mentions

The command may also be a Python module in which case it is run in the same
Python interpreter as this script (a small optimization to avoid restarting the
Python interpreter).

but I don't see that in the implementation.

[jdemeyer]

comment:8

Replying to @jdemeyer:

Why did you remove sys.stderr.flush()?

See also https://mail.python.org/pipermail/python-list/2015-November/698421.html

It seems that Python 2 has unbuffered sys.stderr by default, which means that the flushing is not needed.

But this was changed in Python 3, so the flushing would be required there.

[embray]

comment:9

Ah, good catch. I had mistakenly assume that the print() function would automatically flush by default.

As for being "only for packages depending on Python" that has me wondering now what the best thing would be to do with this. Maybe it should be moved to build/bin. I need to use it for basically all packages, and the fact that it uses Python is not an impediment to that, since build tools are allowed to use the system Python (and in fact require it). But this script is also used at runtime (just for pip install, currently), in which case it should use Sage's Python. Any ideas?

[sagetrac-git]

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

60c7670	removed false statement from the docstring
e960c8a	Turns out print() does not flush I/O by default

[sagetrac-git]

Changed commit from 5e89b68 to e960c8a

[jdemeyer]

comment:11

Replying to @embray:

the fact that it uses Python is not an impediment to that, since build tools are allowed to use the system Python (and in fact require it).

If I recall correctly, we require system Python 2.6 or later, which means that you can't use argparse.

[jdemeyer]

comment:12

Replying to @sagetrac-git:

e960c8a	Turns out print() does not flush I/O by default

The flush keyword is Python 3 only.

[embray]

comment:13

D'oh, how annoying. In that case I'll just switch it back to sys.stderr.write....

[embray]

comment:14

Replying to @jdemeyer:

Replying to @embray:

the fact that it uses Python is not an impediment to that, since build tools are allowed to use the system Python (and in fact require it).

If I recall correctly, we require system Python 2.6 or later, which means that you can't use argparse.

That's not an issue. We already addressed this some time ago... https://github.com/sagemath/sagetrac-mirror/blob/develop/build/sage_bootstrap/compat?id=3faf8d0519918feb308b3bb56fbe36e0a0c2a290

The question is, should the script live in build/bin or src/bin, and what should go in the shebang line? This is unclear for Python scripts that we want to work at build time and run time...

[sagetrac-git]

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

6e3333d

Annoyingly, the flush kwarg for the print_function does not exist in Python 2...

[jdemeyer]

comment:16

Replying to @embray:

I need to use it for basically all packages

What is your use case?

[embray]

comment:17

See description.

[embray]

comment:18

To clarify, the sage-rebase command opens, and often writes to, every DLL installed in $SAGE_LOCAL. It is run at the end of every package build. If two packages complete build/installation at the same time, then there is a race to run sage-rebase, and the second one can fail, causing the build to halt. This especially happens while installing the Python packages because there are a large number of packages being installed quickly one after the other. But it can happen (and has happened) in principle any other time. So I need to put a lock around sage-rebase to prevent it from being run simultaneously for multiple packages.

[jdemeyer]

comment:19

Replying to @embray:

what should go in the shebang line?

If you intend to use the system Python if applicable, it should be #!/usr/bin/env python.

[embray]

comment:20

Replying to @jdemeyer:

Replying to @embray:

what should go in the shebang line?

If you intend to use the system Python if applicable, it should be #!/usr/bin/env python.

But that's the problem, because if it's intended to be used at runtime it should in that case be using Sage's Python (I guess?), and with SAGE_PYTHON3=yes this is incorrect since python still means python2.7.

That said, the original use case for this was with pip, and running pip is still arguably a "build" action, so maybe this should just be moved to build/bin to emphasize that it's a build tool. It can use the system python by default and/or Sage's Python 2 because ultimately it doesn't matter what Python this is run with since it just execs the program it wraps.

[sagetrac-git]

Changed commit from 6e3333d to 3ec24bc

[sagetrac-git]

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

3ec24bc

Moved to build/bin to emphasize that this is primarily a build utility (under its current use cases).

[embray]

comment:22

You've convinced me (and/or I've convinced myself) that this is the best approach.

[jdemeyer]

comment:23

Replying to @embray:

That's not an issue. We already addressed this some time ago... https://github.com/sagemath/sagetrac-mirror/blob/develop/build/sage_bootstrap/compat?id=3faf8d0519918feb308b3bb56fbe36e0a0c2a290

But then you need to ensure that your script can find this argparse module.

[embray]

comment:24

Okay, I was under the mistaken assumption, for some reason, that this was added to PYTHONPATH during builds.

[sagetrac-git]

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

18f9454

Move the contents of sage-flock into the sage_bootstrap package itself, and replace the sage-flock script with a boilerplate script to import and run it from sage_bootstrap

[sagetrac-git]

Changed commit from 3ec24bc to 18f9454

[sagetrac-git]

Changed commit from 18f9454 to b6e4a62

[sagetrac-git]

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

b6e4a62

Merely being part of the sage_bootstrap package doesn't mean we don't still have to manually try to import argparse from the bundled compat module

[embray]

comment:29

This is increasingly critical since not having it can lead to failures even in the patchbot base build, causing the patchbot to exit.

[vbraun]

Changed branch from u/embray/build/lockfile to b6e4a62