-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
bpo-40747: Make py_version_nodot 3_10 not 310 #20333
Conversation
Hmmm, this change may have some other unforeseen consequences. For instance, IIRC this affects the path (indeed, there is a test failing for Windows). Someone more familiar with packaging should take a look as well to make sure this does not have some other unwanted consequences. CC: @brettcannon |
Should probably have @zooba take a look as well. I don't see where the short no dot variable from either copy of sysconfig is used in Unix-y builds of cpython. I've done a quick macOS framework style build (--enable-framework) and I haven't noticed any obvious change in paths or any test suite failures. I'd be inclined to not make the change though unless it is likely to cause problems for downstream packages that do use the variable. But is it really ambiguous? cpython versions are always of the form 3.x, never 3.x.y, as far as constructing path names in builds, AFAIK. That is, for unix-y builds using autoconf. @zooba could speak for the Windows build system. And I'm assuming by the time we get to Python 9 there will be bigger issues :) |
It's ambiguous from the perspective of what is |
The FAQ for PEP 425 has this:
I think "major release" in this context means 3.4, 3.5, 3.6, ... so we are about to hit a 3-digit major release. |
Tests pass, including one new test for consistency. |
configure.ac
Outdated
AC_MSG_RESULT($SOABI) | ||
|
||
# Release and debug (Py_DEBUG) ABI are compatible, but not Py_TRACE_REFS ABI | ||
if test "$Py_DEBUG" = 'true' -a "$with_trace_refs" != "yes"; then | ||
# Similar to SOABI but remove "d" flag from ABIFLAGS | ||
AC_SUBST(ALT_SOABI) | ||
ALT_SOABI='cpython-'`echo $VERSION | tr -d .``echo $ABIFLAGS | tr -d d`${PLATFORM_TRIPLET:+-$PLATFORM_TRIPLET} | ||
ALT_SOABI='cpython-'`echo $VERSION | tr -d "." "_"``echo $ABIFLAGS | tr -d d`${PLATFORM_TRIPLET:+-$PLATFORM_TRIPLET} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On macOS, at least,
$ ./configure --with-pydebug
[...]
checking SOABI... cpython-3_10d-darwin
usage: tr [-Ccsu] string1 string2
tr [-Ccu] -d string1
tr [-Ccu] -s string1
tr [-Ccu] -ds string1 string2
checking LDVERSION... $(VERSION)$(ABIFLAGS)
[...]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh. Strange it passes CI. Is the change on line 4740 also problematic? I cannot reproduce, could you try removing the "
so that the "." "_"
becomes . _
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tr
error doesn't cause the configure run to fail. It happens on the Linux CI runs, too: from the Github Ubuntu test:
676 checking SOABI... cpython-3_10d-x86_64-linux-gnu
677 tr: extra operand '_'
678 Only one string may be given when deleting without squeezing repeats.
679 Try 'tr --help' for more information.
680 checking LDVERSION... $(VERSION)$(ABIFLAGS)
(You don't see it in the log for the most recent Travis CI run because it appears that the build environment for the PR is cached and it didn't rerun configure.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed by removing the -d from the tr command. Checked it by rerunning autoconf
to recreate configure
from configure.am
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(FWIW the error makes sense: tr is either original replacement
or -d original
to delete)
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
I have made the requested changes; please review again |
Thanks for making the requested changes! @ned-deily: please review the changes made to this pull request. |
I am fine with closing this, but then pypa/packaging should align itself with that approach. |
I searched Doc, Tools/msi, and PC/layout for |
somewhat surprisingly, CI passed with the MSI changes |
A python 3.10 wheel would produce a wheel with the platform tag as `cp310`, which would be invalid, as only 3_10 is accepted by the wheel package. See: pypa/packaging#308 pypa/wheel#354 python/cpython#20333
Unfortunately, this issue is also blocking running the |
We should aim to resolve this by the end of the core dev sprint so we can unblock the rest of the ecosystem. |
For those of us who didn't know, it's next week:
Sounds good! Should it be added to https://python-core-sprint-2020.readthedocs.io/projects.html? |
@hugovk nah, it will just be something that some of us go and discuss on the side. |
@hugovk I decided not to risk forgetting, so I went ahead and added it to the TODO list. Thanks for the idea! (Currently the build has not happened yet, so don't be shocked if you don't see it at the bottom.) |
@pablogsal downgrading to |
Thanks @brandtbucher ! Unfortunately, we were working on the benchmarks for the speed.python.org server and downgrading uses a different versions of the packages being tested and therefore the generated data cannot be compared with the previous data 🙁 |
For fellow core devs: feel free to add/remove yourself from https://python-core-sprint-2020.readthedocs.io/projects.html#wheel-interpreter-naming-for-3-10 and I will try to schedule a meeting when we can chat about this. I will wait until Monday to schedule based on who does (not) want to attend to make it timezone friendly (e.g. Pablo I know is London and I'm Vancouver). |
My sleep schedule is already destroyed and I am a night person, so feel free to push the meeting closer to your timezone if needed 😉 |
@@ -10,7 +10,7 @@ | |||
</PropertyGroup> | |||
<Import Project="..\msi.props" /> | |||
<PropertyGroup> | |||
<DocFilename>python$(MajorVersionNumber)$(MinorVersionNumber)$(MicroVersionNumber)$(ReleaseLevelName).chm</DocFilename> | |||
<DocFilename>python$(MajorVersionNumber)_$(MinorVersionNumber)$(MicroVersionNumber)$(ReleaseLevelName).chm</DocFilename> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably separate the minor and micro version of the doc filename too - python3_10_0.chm
rather than python3_100.chm
. The actual change ought to be somewhere under Doc, IIRC.
FYI, I've built and run with the PR merged up to the current HEAD of master with both a vanilla installed Linux config and my standard macOS installer build. In both cases, no test failures due to the PR were noted. In both cases, I also ran
I have no opinion ATM of the importance of either one, just reporting them. |
@mattip we discussed this at the sprint and decided that once Steve's concerns are addressed and I write a quick PEP this week on the chnage we can merge the PR. |
The docs filename change seems to be at line 108 of https://github.com/mattip/cpython/blob/py_version_nodot/Doc/conf.py Turns out the MSI build in PR is using the "friendly" option that doesn't fail when the doc file is missing, but this line shows that it wasn't found. I think we should be fine to just allow the extra dots in the .chm name though, rather than going to underscores. And actually, I think we should try going to dots in the DLL name as well. Windows should be able to handle it, and it's more consistent between And if we're going to cause (a few) people to have to update their code anyway, we may as well end up somewhere more consistent :) |
I'm not sure I understand what still needs to be changed. Could a reviewer who does understand push changes to this PR? |
I can push changes, just didn't want to intrude since you've been active. |
@zooba or anyone else, please feel free to make any necessary changes to move this forward |
I've had to reopen as #22858 with a branch on my own fork, since I can only edit mattip's through the web UI (which, as you'll see, was not going to be worthwhile). |
Fixes pypa/packaging#308, pypa/wheel#354, and maybe pypa/pip#8312.
The current value of
310
is ambiguous so use3_10
instead. While this may fixpip
andwheel.bdist_wheel
, it has the potential to break other projects that depend on_PY_VERSION_SHORT_NO_DOT
being 2 characters or that calculate the value via'%d%d' % sys.version_info[:2]
themselves.https://bugs.python.org/issue40747