Check in prepare_linked_requirement for an existing directory used by --src is setuptools-specific

[pfmoore]

Environment

• pip version: 20.1.1
• Python version: all
• OS: all

Description
In pip._internal.operations.prepare, the method prepare_linked_requirement checks for a previous build directory by looking for setup.py. This check is setuptools-specific, and won't work for general PEP 517 backends.

As a result of various refactorings, this code is only called when --build is specified. In all other cases, we use a temporary build directory.

Expected behavior
In practice, it's not clear that there is any benefit in trying to use the build directory here. We should drop this check and just use a temporary directory, so there's no chance of a clash.

Additional discussion occurred in #8283, here and here in particular.

Update

The --build-directory option is now removed. This means that the only place where pip still uses a TemporaryDirectory with an explicit pre-defined path is in support of editable installs, which are setuptools-specific. So checking for setup.py is acceptable in that context.

Someone may wish to consider how we handle of the --src option at some point. This is what leads to TemporaryDirectory needing a path argument, and ultimately to this check, and it may be possible to refactor that code path to avoid using the TemporaryDirectory class for something which definitely isn't temporary...

When editable support is standardised, pip will need to implement the new standard. At that point, all of the code relating to editable installs will be reviewed, and this check (and the use of TemporaryDirectory) can be cleaned up then, if it still remains.

[sbidoul]

I also suspect that --build is only used for debugging (and it apparently applies to dependencies only in my test it seems to be ignored for user supplied requirements).

So I'd agree to remove all use of that option, make it a hidden option, and warn if it is used, orienting users to $TMPDIR and --no-clean for diagnostics.

[pradyunsg]

I'm on board for deprecating --build in 20.2, and getting rid of it ASAP. :)

[sbidoul]

Noting that --build was once deprecated, then undeprecated in #2062.

[uranusjr]

Also noting so others don’t need to trace the discussion again: Unfortunately no reasons were given for why the deprecation was reversed. It was also mentioned in #906 (comment) that the feature can be adequately replaced by setting environment variables.

[brainwane]

It would be great if someone could help with this since Pradyun has a lot to do in the next week -- @pfmoore @xavfernandez @chrahunt could one of you step up?

[pfmoore]

@brainwane I'm not sure what the actionable item is here. The issue is saying "always use a temporary directory" but the discussion seemed to veer off into "deprecate --build".

I might be able to take a look at the former over the weekend, but I don't want to get sucked into the politics of deprecating --build (I have too many other commitments and not enough time already!)

Also, I'm not clear why this is a priority? It seems like more of a "nice to have" activity. I'll try to check the linked issues later today to see if that gives me a clue.

[pradyunsg]

@pfmoore This is already deprecated -- we basically need to remove the deprecation for this that's marked gone_in="20.3" and the corresponding code that's deprecated. Or, we can kick the can to 21.0 if no one can come around to implementing this. :)

[uranusjr]

Not related to the above discussion, but I noticed this issue is getting into the #8368 trouble, where unsuspected users link and comment here when they see any compilation failure unrelated to the --build flag. Should we use the similar solution that only displays the deprecation warning if the build is successful?

[pradyunsg]

@uranusjr I don't think we really need to do anything on that front here, but we should keep that in mind for future deprecations.

[pfmoore]

@pradyunsg Thanks, I'll try to check that out over the weekend.

[brainwane]

Great - thank you, @pfmoore, that helps a lot.

[pfmoore]

#9049 submitted for this. It removes the --build-dir option as per the deprecation, but it doesn't actually remove the setup.py check that started this discussion, because as far as I can tell, that is still used in one case, when we are doing an editable install (where --src controls the location of the build directory, not --build-dir).

Checking setup.py is valid there, as editable installs are only handled in setuptools at the moment, so a setuptools-specific check is valid. But we now have a bunch of machinery allowing a path to be specified for TemporaryDirectory whose sole purpose is to support an editable checkout directory (which frankly, isn't something I'd describe as "temporary" 🙁).

We should probably refactor that code at some stage so we can rip out the path argument for TemporaryDirectory, but I don't want to go digging into the intricacies of editable installs right now. I don't know whether we:

1. Leave this issue open to track that point
2. Open a new issue to track it
3. Don't bother having an issue at all, and just let it be part of a future rework of editables

I'm in favour of (1) as the simplest option, but if someone prefers (2) I'm OK with that.

[pradyunsg]

#9049 is merged, and... (1) sounds good to me. I'll let @pfmoore decide if there's any retitling/more-info-in-first-post needed on this issue. :)

[abitrolly]

Can anybody open a master issue explaining pros and cons for deprecating --build? For a reference that is shown in a DEPRECATION notice to every pip user who uses it, I find the explanation in this issue quite deep in technical details and not sufficient for general understanding. Particularly, it touches a narrow part of --build usage that sounds like one specific case of setuptools behaviour. And it doesn't list alternative use cases, where --build is currently used, such as debugging pip builds.

PRO --build

Using TMP/TEMP or TMPDIR environment variable is fine as long as it is documented in pip -h alongside other debug options, such as -v and --no-clean. The downside of such vars is that if they are misspelled or a wrong var is set, there is no way for pip to detect that error, while in case of command line option the error is reported automatically. That's the main argument for preserving --build.

The minor argument for that is to implement a build dir management class. Right now every pip component manages its temp/build dir separately, and not every of them honors --no-clean option. Each build right now also creates several dirs in /tmp/pip- and after several builds comparing the outputs between them becomes a disaster. It would be more useful to have a single temp /tmp/pip-$HASH dir for each build with subdirs related to each component.

[kushaldas]

We were using --build to create reproducible wheels as shown in https://github.com/freedomofpress/securedrop-debian-packaging/blob/main/scripts/build-sync-wheels#L99-L100. can anyone please tell me how to do that with the latest pip?

Edit: I am trying now with TEMPDIR, TMP, TEMP, TMPDIR on Debian Buster, all 4 set to a static directory, and still I can see /tmp/pip-wheel-xxxxxxxx was used :( I think pip is still using a temporary directory to build in what ever system is pointing to as primary TEMPORARY DIRECTORY. That is causing the break in reproducibility.

[uranusjr]

What platform are you on? Different systems use different environment variables to control the temporary directory's location.

[kushaldas]

What platform are you on? Different systems use different environment variables to control the temporary directory's location.

Linux in my case. But, we should document how to do this in every environment properly.

[uranusjr]

IIRC most Linux distros use TMPDIR (no E), which is the standard POSIX environ. Documentation contribution is very much welcomed, but listing the correct environment for Linux flavours would likely cost much time because there are so many variants.

[jordansissel]

I also suspect that --build is only used for debugging

fpm uses --build. It's possible that fpm's use of --build is wrong somehow, but it's been used to capture pip install results since 2016

Related: jordansissel/fpm#1831

[pradyunsg]

Closing this to consolidate into #7555.

[pradyunsg]

Okay, no, this got fixed in #10082, where this check was switched to a much more general check that is about "is this directory installable".