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

Bump version to reopen for development #5853

Merged
merged 2 commits into from
Oct 8, 2018

Conversation

pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Oct 5, 2018

Toward #5782

@pradyunsg pradyunsg added the skip news Does not need a NEWS file entry (eg: trivial changes) label Oct 5, 2018
@pradyunsg
Copy link
Member Author

I expect the tests to fail due to #5542 changing it so that we have to remove the deprecated items before we continue development for the next release.

@pradyunsg
Copy link
Member Author

3 tests failed, and dependency-links are disabled by the deprecation helper now.

Damn, that's extremely poorly tested.

@pradyunsg
Copy link
Member Author

@pypa/pip-committers Someone will have to put in the work to actually remove dependency-links support from pip before we can reopen master for development. :/

I'll try to get around to that tonight, assuming I've enough energy after PyCon India.

@benoit-pierre
Copy link
Member

How about extending the deprecation period instead?

@pradyunsg
Copy link
Member Author

That would work too, but I don't want to push the removal date; even temporarily.

@benoit-pierre
Copy link
Member

My 2 cents: PEP 508 URLs are no replacement for dependency links, because of the lack of version specifiers.

For example, with dependency links, you can push a package on PyPI, with dependencies on other PyPI project, but the option to use a patched version for some of those dependencies (with some extra bug fixes) when using --process-dependency-links. I've use that myself on a project depending on PyObjc because the delay between releases is so long.

Additionally, the lack of version specifiers mean there's no way for pip to know if a existing installation is compatible or not: this is problematic when upgrading a project depending on another through a PEP 508 direct URL, but also make sharing a such a dependency between projects problematic.

And finally, dependency links are opt-in, and usable on PyPI. But PEP 508 URLs are forbidden by pip during install for projects originating from PyPI: for "security reasons". This, to me, does not really make sense: it's not like installing from PyPI only is secure!

@cjerdonek
Copy link
Member

Something doesn't seem right about the development workflow if the deprecation tasks have to be addressed before even starting work on the development branch. It seems like those tasks are things that should be done while working on the development branch (as part of the other tasks that need to be done for the next release). A couple reasons are that (1) it might take time (e.g. for reviews, etc), and (2) we might want to break it up into more than one PR (e.g. for different things being deprecated). Certain individuals might be better equipped to address the deprecation in one feature but not another.

@benoit-pierre
Copy link
Member

benoit-pierre commented Oct 7, 2018

The version check could be changed to:

current_version >= gone_in and not (current_version.is_prerelease and current_version.base_version == gone_in)

@cjerdonek
Copy link
Member

A couple concrete suggestions: (1) bump the version to a development version that doesn't trigger the deprecation (e.g. 18.2.dev0?), or (2) alter the decorator not to fail for this hard-coded release number. When the deprecated items are removed, depending on whether (1) or (2) is used, the version can be bumped to the preferred number, or the decorator changed back.

@benoit-pierre
Copy link
Member

 src/pip/_internal/index.py             | 2 +-
 src/pip/_internal/operations/freeze.py | 2 +-
 src/pip/_internal/utils/deprecation.py | 8 ++++++--
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git i/src/pip/_internal/index.py w/src/pip/_internal/index.py
index 8c2f24f1..5f3c2d62 100644
--- i/src/pip/_internal/index.py
+++ w/src/pip/_internal/index.py
@@ -293,7 +293,7 @@ def add_dependency_links(self, links):
                 "Dependency Links processing has been deprecated and will be "
                 "removed in a future release.",
                 replacement="PEP 508 URL dependencies",
-                gone_in="18.2",
+                gone_in="19",
                 issue=4187,
             )
             self.dependency_links.extend(links)
diff --git i/src/pip/_internal/operations/freeze.py w/src/pip/_internal/operations/freeze.py
index beb2feb8..26d2e3b8 100644
--- i/src/pip/_internal/operations/freeze.py
+++ w/src/pip/_internal/operations/freeze.py
@@ -227,7 +227,7 @@ def _init_args_from_dist(cls, dist, dependency_links):
                         "SVN editable detection based on dependency links "
                         "will be dropped in the future.",
                         replacement=None,
-                        gone_in="18.2",
+                        gone_in="19",
                         issue=4187,
                     )
                     comments.append(
diff --git i/src/pip/_internal/utils/deprecation.py w/src/pip/_internal/utils/deprecation.py
index bd744cf2..c5b4dc4b 100644
--- i/src/pip/_internal/utils/deprecation.py
+++ w/src/pip/_internal/utils/deprecation.py
@@ -84,6 +84,10 @@ def deprecated(reason, replacement, gone_in, issue=None):
         message += " You can find discussion regarding this at {}.".format(url)
 
     # Raise as an error if it has to be removed.
-    if gone_in is not None and parse(current_version) >= parse(gone_in):
-        raise PipDeprecationWarning(message)
+    if gone_in is not None:
+        current = parse(current_version)
+        gone_in = parse(gone_in)
+        if current >= gone_in and not (current.is_prerelease and
+                                       current.base_version == gone_in):
+            raise PipDeprecationWarning(message)
     warnings.warn(message, category=PipDeprecationWarning, stacklevel=2)

@cjerdonek
Copy link
Member

Doesn't >= already take into account pre-releases?

>>> from packaging.version import parse
>>> parse("19.0.dev0") >= parse("19")
False

@benoit-pierre
Copy link
Member

Yep! I don't know how I got the test wrong. So just patch the gone_ins to 19, as that will be the next release number, no?

@cjerdonek
Copy link
Member

Yeah, that would work. (Or bump the release number to "18.2.dev0" for now.) Let's wait to hear @pradyunsg's thoughts, too.

@cjerdonek
Copy link
Member

FYI, I would like to do the deprecation in freeze.py when the time comes since I have an outstanding PR (PR #5837) that overlaps the affected code.

@pradyunsg
Copy link
Member Author

Yeah, that would work.

Yeah.

I feel it'd be nice if dropping the .dev segment is not a change that has changes in behavior, since otherwise the release process becomes multi-hour task with our current CI setup:

  • do pre-release stuff
  • drop the dev version
  • make a PR + wait + merge
  • bump the version
  • make a PR + wait + merge
  • continue development

We can discuss this in a dedicated issue later though. Let's bump the numbers to 19 for now and unlock our workflow though.

@pradyunsg
Copy link
Member Author

PEP 508 URLs are no replacement for dependency links, because of the lack of version specifiers.

@benoit-pierre It'd probably be a good idea to have a single point for this discussion, since it's something you've flagged multiple times but I'm not sure it's been directly discussed in a dedicated location. If there isn't one (I'm not upto date yet with the recent events in this tracker), could you open a new issue to discuss this?

@cjerdonek
Copy link
Member

@pradyunsg Mind if I merge this, or can you?

@pradyunsg
Copy link
Member Author

@cjerdonek You can go ahead and merge. :)

@cjerdonek
Copy link
Member

Thanks!

@cjerdonek cjerdonek merged commit 9d070c0 into pypa:master Oct 8, 2018
@pradyunsg pradyunsg deleted the reopen-for-development branch October 8, 2018 17:32
@pradyunsg pradyunsg mentioned this pull request Oct 8, 2018
@lock
Copy link

lock bot commented May 31, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label May 31, 2019
@lock lock bot locked as resolved and limited conversation to collaborators May 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants