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

Improve the phrasing of in-tree build deprecation warning #10129

Merged
merged 4 commits into from
Jul 30, 2021

Conversation

maresb
Copy link
Contributor

@maresb maresb commented Jul 4, 2021

Closes #10128

@maresb maresb changed the title Patch 1 Improve the phrasing of in-tree build deprecation warning Jul 4, 2021
@maresb
Copy link
Contributor Author

maresb commented Jul 4, 2021

I'm not sure if my news entry type for "doc" is correct since I'm adjusting the code... (I'm new to towncrier.)

Let me know if you'd like me to change it or mark as trivial or whatever.

@pradyunsg
Copy link
Member

pradyunsg commented Jul 4, 2021

doc works! :) I changed my mind after re-reading the original post -- let's make it a removal.

The old message is:

DEPRECATION: A future pip version will change local packages to be built in-place without first copying to a temporary directory. We recommend you use --use-feature=in-tree-build to test your packages with this new behavior before it becomes the default.
pip 21.3 will remove support for this functionality. You can find discussion regarding this at #7555.

This PR proposes to change it to:

DEPRECATION: A future pip version will build local packages in-place ("in-tree") without first copying to a temporary directory. Before this new behavior becomes the default, we recommend testing your packages using the option. Regarding this temporarily available command-line option,
pip 21.3 will remove support for this functionality. You can find discussion regarding this at {link}.


Some part of me wants to change it to something like:

DEPRECATION: pip copied a local directory before performing a build. This behaviour will change to stop copying and performing a build "in-tree".
pip 21.3 will enforce this behaviour change. You can use --use-feature=in-tree-build to test the upcoming behaviour.
Discussion regarding this change is at {link}.

This would, however, be a two-fold change -- partly here and partly in the deprecated function.


If you want to avoid increasing the scope of this PR significantly, let's do something like:

DEPRECATION: pip has copied a package to a temporary directory, to build an installable artifact from it. This behaviour is going to change, to instead perform the build without copying in the original package source tree ("in-tree" build). This upcoming behaviour can be tested using --use-feature=in-tree-build. It is recommended to ensure that packages work with the upcoming behaviour and that they do not depend on pip's current copying behaviour.
pip 21.3 will remove support for this functionality. You can find discussion regarding this at {link}.

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested rewording above for the message, and for the NEWS fragment below.

news/10128.doc.rst Outdated Show resolved Hide resolved
@maresb
Copy link
Contributor Author

maresb commented Jul 4, 2021

Hi there @pradyunsg, thanks so much for the feedback!

I agree with the spirit of your suggestions. I wonder if we can make it a bit more concise. I'm posting an updated suggestion...

@maresb
Copy link
Contributor Author

maresb commented Jul 4, 2021

@pradyunsg, do you understand the intention with regards to the --use-feature=in-tree-build? Will that argument be removed in 21.3 or will the build behavior switch from out-of-tree to in-tree in 21.3?

@maresb
Copy link
Contributor Author

maresb commented Jul 4, 2021

British spelling is "behaviour" while American spelling is "behavior". The original message was American. Did you intend to switch to British?

Here's my next iteration:

DEPRECATION: pip currently copies the source tree into a temporary directory before building it. In the future, pip will build packages in-place in the original source tree ("in-tree" build). It is recommended to verify that your packages are compatible with the upcoming behavior by testing them with the --use-feature=in-tree-build argument. Regarding the default out-of-tree build behavior,
pip 21.3 will remove support for this functionality. You can find discussion regarding this at {link}.

Does this read well to you?

@maresb
Copy link
Contributor Author

maresb commented Jul 4, 2021

@pradyunsg I think I may have misunderstood the deprecation message earlier. Will --use-feature=in-tree-build still work after the defaults change?

If so, I think this is better:

DEPRECATION: pip currently copies the source tree into a temporary directory before building it. In the future, pip will build packages in-place within the original source tree ("in-tree build"). Before the default behavior changes, we recommend testing your packages by adding the --use-feature=in-tree-build argument. Regarding the current out-of-tree default build behavior,
pip 21.3 will remove support for this functionality. You can find discussion regarding this at {link}.

@maresb
Copy link
Contributor Author

maresb commented Jul 7, 2021

Hello @pradyunsg, I have updated the text of my message according to the last suggestion I made, and squashed my commits.

The original intention of the deprecation message is still unclear to me. I think it is either

  1. The --use-feature=in-tree-build argument will be deprecated in pip 21.3
  2. The out-of-tree build behavior will be deprecated in pip 21.3

I originally believed that it was 1. Now that I see how the deprecated function works, I think it's more likely that the intention was 2, and the deprecated function happened to format the text in an unfortunate way so that 1 seemed to be intended.

Am I correct that 2 is the correct interpretation of the deprecation warning? And that the --use-feature=in-tree-build will be available long-term? Otherwise my text will require further revisions.

And finally, are you satisfied with my phrasing? I tried to address your concerns, but if you think the text can still be improved, please let me know!

If the following conditions are true:

  • The correct intention of the deprecation message was 2
  • You are satisfied with the text

then as far as I can tell from my side, this is ready for merge and release.

@pradyunsg pradyunsg added this to the 21.2 milestone Jul 7, 2021
@pradyunsg
Copy link
Member

pradyunsg commented Jul 7, 2021

Both are partially correct. The plan is:

Now: no flag prints deprecation warning and old behavior. --use-feature=in-tree-build uses new behavior.
20.3: no flag uses new behavior. --use-feature=in-tree-build is a no-op. There's a new --use-deprecated={something} that uses old behavior.
Future: no flag uses new behavior. Both the additional flags are removed and result in errors.

The current message is for conveying "hey, this copying thing is gonna change. Make sure your package works without copying with this option, before the default changes".

@pradyunsg
Copy link
Member

pradyunsg commented Jul 7, 2021

Regarding the current out-of-tree default build behavior,
pip 21.3 will remove support for this functionality. You can find discussion regarding this at {link}.

I'd like to avoid the "regarding..." phrasing, but, eh, I don't feel strongly about this. I probably feel more strongly that we shouldn't have a newline though. We're already at a long-enough message here that there's wrapping.

@maresb
Copy link
Contributor Author

maresb commented Jul 7, 2021

Ah, ok, thank you for clarifying. Then there will be no long-term way to suppress this deprecation warning.

@maresb
Copy link
Contributor Author

maresb commented Jul 7, 2021

I'm a bit concerned that people will try to make the deprecation warning go away, add --use-feature=in-tree-build to their scripts, forget about it, and get burnt when it raises an exception in the future.

I also found the \n weird, I'll happily remove it.

I'm not so fond of the "Regarding..." myself, but I feel like it's important to clarify the meaning of the word "this". I'd be happy for an alternative way to accomplish that. I'm actually leaning towards opening another issue about pip._internal.utils.deprecation:deprecated where I introduce DEPRECATION_GONE_IN_MESSAGE = "pip {} will remove support for this functionality." and add a new kwarg gone_in_message=DEPRECATION_GONE_IN_MESSAGE so that we can avoid the awkward wording here.

I don't like how long this message is, but at least it's specific.

My latest attempt:

pip currently copies the source tree into a temporary directory before building it. pip 21.3 will build packages in-place within the original source tree ("in-tree build"). This new behavior can be tested with the --use-feature=in-tree-build flag. Future versions of pip will remove support for out-of-tree builds, and for this testing flag. You can find discussion regarding this at {link}.

@uranusjr
Copy link
Member

I'm a bit concerned that people will try to make the deprecation warning go away, add --use-feature=in-tree-build to their scripts, forget about it, and get burnt when it raises an exception in the future.

The --use-feature rule says that before a feature flag can be removed, we must do at least one major release that makes the feature flag a no-nop with a warning message, so this shouldn’t be an issue. If a user add --use-feature=in-tree-build now and forget about it, they’ll be notified when we make in-tree build the default and have at least three months to remove the flag.

@uranusjr
Copy link
Member

So what is the next step for this? Would it be possible to get this ready for 21.2 this month?

@maresb
Copy link
Contributor Author

maresb commented Jul 23, 2021

@uranusjr waiting on #10167. If I am allowed to add a new sentence template there regarding --use-feature= I think it will lead to a more natural solution here.

I really hope that we make it in time for 21.2.

@uranusjr
Copy link
Member

It seems to me the discussion for #10167 is not going to reach a conclusion immediately, so I'm releasing 21.2 without this.

@uranusjr uranusjr removed this from the 21.2 milestone Jul 24, 2021
@maresb
Copy link
Contributor Author

maresb commented Jul 27, 2021

This new fix will work once rebased onto #10167 so that the feature_flag= kwarg exists.

@maresb
Copy link
Contributor Author

maresb commented Jul 29, 2021

@uranusjr and @pradyunsg, this should now work thanks to #10167.

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but suggesting one minor style change. :)

@maresb
Copy link
Contributor Author

maresb commented Jul 29, 2021

@pradyunsg I'm looking for the style change you'd like to suggest, but I'm not finding it. Could you please let me know again what exactly you'd like changed?

@maresb
Copy link
Contributor Author

maresb commented Jul 29, 2021

Perhaps it vanished after you "approved" the changes? (weird GitHub stuff?)

@pradyunsg
Copy link
Member

weird GitHub stuff?

Indeed!

@maresb
Copy link
Contributor Author

maresb commented Jul 29, 2021

Got it, thanks @pradyunsg! It looks nicer this way.

@maresb
Copy link
Contributor Author

maresb commented Jul 29, 2021

Oops, the indent pushed the line over the limit...

@maresb
Copy link
Contributor Author

maresb commented Jul 29, 2021

@pradyunsg and @uranusjr, all tests are passing. Ready to merge? Thanks!

@uranusjr uranusjr merged commit 6da0819 into pypa:main Jul 30, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecation warning about in-place builds is confusing/ambiguous
3 participants