In D6258#91056, @indygreg wrote:

In D6258#91055, @durin42 wrote:

Argh. This change appears to be wrong (!) so I'm going to drop it.

Eh? I don't see anything wrong with this. Casting a pathlib.Path to str should just work! In fact, it is necessary for older Python 3, as there was a long tail of stdlib functions that didn't accept pathlib.* when those types were first introduced. I believe 3.7 was the first release that fixed them all.

Argh. This change appears to be wrong (!) so I'm going to drop it.

Ugh I'm sorry - I evidently reviewed this and never pushed submit. :(

@lothiraldan do y'all have opinions on this? I'd love to see this get landed.

In D6164#90073, @indygreg wrote:

I would prefer you fix them :)

In D6164#90059, @indygreg wrote:

Was a newer version of this patch with the requested changes going to be uploaded? It's weird to see additional patches in the series without this one updated...

I'm what I'll call an unenthusiastic fan of this: it feels a little gross, but I have no better ideas (in fact, at least part of this interface was my idea...) so I'd like to see this land as it'll open us up to carry fewer painful patches at Google.

In D6164#89916, @indygreg wrote:

In D6164#89915, @durin42 wrote:

In D6164#89914, @indygreg wrote:

In D6164#89913, @durin42 wrote:

How about we add an argument to define the path(s) to the pip requirements file(s) to use? By default, it can use the requirements.txt in the repo. Would that solve your use case?

No, because I have some stuff which (out of tragic necessity at the moment) isn't pip installable, and I have to install it into the virtualenv by banging some rocks together. :(

Bleh. So you do need the power of a full script here.

In D6164#89914, @indygreg wrote:

In D6164#89913, @durin42 wrote:

How about we add an argument to define the path(s) to the pip requirements file(s) to use? By default, it can use the requirements.txt in the repo. Would that solve your use case?

In D6164#89881, @indygreg wrote:

I like the flexibility. But I'm not super keen about the interface here. Using a script to inject custom options seems like it could be useful. But as it is currently implemented, the script simply prints out \0 delimited package names. So one UI wart is --extra-prebuild-script being a somewhat generic name but that script only emits package names. --extra-packages-script would be a better name.

FYI @spectral I uploaded this.

This is Kyle's patch from https://bz.mercurial-scm.org/show_bug.cgi?id=6070 - it feels regrettable but necessary, and anecdotally it seems to work. I'd give it an LGTM stamp here, but I can't since I uploaded it.

In D5994#87506, @martinvonz wrote:

I'd prefer Python accept a tuple of context managers

I think I'd also prefer if our context managers didn't acquire the resource in __init__. That would let us write it with less indentation as:

wlock = wlock or util.nullcontextmanager()
lock = lock or util.nullcontextmanager()
trmanager = pushop.trmanager or util.nullcontextmanager()
with wlock, lock, trmanager:

(In this case we probably could do that anyway since we don't seem to worry about the risk of exceptions from the time we take the lock until we enter the context manager.)

I don't care much either way about the patch itself. I'll wait a little to see if someone else cares more, but otherwise I'll take.

(moving this off the dashboard until it gets updated)