Skip to content

gh-76568: Remove ability to import non-version-tagged C extensions #4943

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

Closed
wants to merge 8 commits into from

Conversation

pitrou
Copy link
Member

@pitrou pitrou commented Dec 20, 2017

This excludes HP-UX, AIX and Cygwin for which we haven't migrated to version-tagged C extension names.

This excludes HP-UX, AIX and Cygwin for which we haven't migrated to
version-tagged C extension names.
@AraHaan
Copy link
Contributor

AraHaan commented Dec 20, 2017

The tests are failing because of the fact that the project settings for all of the c extensions in PCbuild\pcbuild.sln does not contain the tag based on the platform configuration. That will have to change before merging this.

@pitrou
Copy link
Member Author

pitrou commented Dec 20, 2017

@AraHaan you're right. Unfortunately those files are static but the tag is varying (platform-dependent and version-dependent), so I'm not sure how to solve this issue. Perhaps @zooba can help.

@AraHaan
Copy link
Contributor

AraHaan commented Dec 20, 2017

I think the solution is to have python.props sort out the tag and on the static file ext field have .$(PyTag).pyd

@pitrou
Copy link
Member Author

pitrou commented Dec 20, 2017

I'm probably unable to do it myself. Would you like to propose a patch?

@AraHaan
Copy link
Contributor

AraHaan commented Dec 20, 2017

oh, looking at python.props it already has this at line 174~176:

    <!-- The version and platform tag to include in .pyd filenames -->
    <PydTag Condition="$(ArchName) == 'win32'">.cp$(MajorVersionNumber)$(MinorVersionNumber)-win32</PydTag>
    <PydTag Condition="$(ArchName) == 'amd64'">.cp$(MajorVersionNumber)$(MinorVersionNumber)-win_amd64</PydTag>

So basically all I need to do is propose an patch that has .$(PydTag).pyd on all those project files that builds pyd's.

Edit: @pitrou I attached the patch that you can try to apply here on bpo.

@pitrou pitrou requested review from rhettinger, skrah and a team as code owners December 20, 2017 22:04
@pitrou
Copy link
Member Author

pitrou commented Dec 20, 2017

Thanks for the fix! There seems to be two remaining references in PCbuild/prepare_ssl.py and PCbuild/_testconsole.vcxproj. Would welcome your help there too, if you are able to try and test changes on Windows :-)

@pitrou
Copy link
Member Author

pitrou commented Dec 20, 2017

Also, there are now the following warnings when building:

C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\Common7\IDE\VC\VCTargets\Microsoft.CppBuild.targets(1199,5): warning MSB8012: TargetExt(.cp37-win32.pyd) does not match the Linker's OutputFile property value (.pyd). This may cause your project to build incorrectly. To correct this, please make sure that $(OutDir), $(TargetName) and $(TargetExt) property values match the value specified in %(Link.OutputFile). [C:\projects\cpython\PCbuild\_hashlib.vcxproj]

@AraHaan
Copy link
Contributor

AraHaan commented Dec 20, 2017

Yeah, I just seen those warnings too.

@pitrou pitrou requested a review from a team December 20, 2017 23:18
@AraHaan
Copy link
Contributor

AraHaan commented Dec 21, 2017

Seems like test_ssl is crashing too.

@pitrou
Copy link
Member Author

pitrou commented Dec 21, 2017

I have now reverted the Windows changes. This is now a very simple PR for POSIX systems only.

@AraHaan
Copy link
Contributor

AraHaan commented Dec 21, 2017

Alright, I will will work on the Windows changes eventually though to see if we can get it all to work on Windows too.

Also @pitrou you could have done an

git branch backup
git checkout master
git branch -D remove_untagged_so_import
git branch remove_untagged_so_import
git checkout remove_untagged_so_import
git cherry-pick backup
git branch -D backup
git push --force

That is what I do to avoid the useless merge branch '%s' into %s commits.

@pitrou
Copy link
Member Author

pitrou commented Dec 21, 2017

@AraHaan, please @zooba's message at https://bugs.python.org/issue32387#msg308878 before attempting anything. We may not want to do any changes on the Windows side.

@AraHaan
Copy link
Contributor

AraHaan commented Dec 21, 2017

Alright, just read it, so the Windows code that was changed here was causing some conflicts.

@ned-deily
Copy link
Member

See additional comments on bpo-32387.

@brettcannon
Copy link
Member

What's the status of this PR?

@ncoghlan
Copy link
Contributor

ncoghlan commented Jun 9, 2018

At a technical level, it's a straightforward change, but at a design level, it isn't clear that it's a change worth making given the compatibility risks associated with it: https://bugs.python.org/issue32387#msg311309

So the PR itself is stalled until the design question has been addressed.

@brettcannon
Copy link
Member

Flagging this a DO-NOT-MERGE until the discussion on the issue tracker is resolved.

@brettcannon brettcannon removed the request for review from a team April 17, 2019 22:44
@brettcannon
Copy link
Member

Removing the import team from reviewing as the appropriateness of this PR has not been decided yet. We can be added back when it's ready to go.

@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Aug 13, 2022
@AraHaan
Copy link
Contributor

AraHaan commented Aug 13, 2022

I think that the ability to import non-version tagged C extensions should be supported still (for now) at least until all places that generate the extensions get updated to always tag them.

@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Aug 14, 2022
@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Feb 21, 2023
@arhadthedev
Copy link
Member

At a technical level, it's a straightforward change, but at a design level, it isn't clear that it's a change worth making given the compatibility risks associated with it: https://bugs.python.org/issue32387#msg311309

So the PR itself is stalled until the design question has been addressed.

Five years passed with no final decision (covering versions 3.5-3.12). Should we recognize that the existing approach is good enough and close this PR and its parent issue for good?

BTW, the last message in the issue is #76568 (comment):

I think it'd be worth doing on POSIX systems for some 3.8 alpha/beta release cycles before making a final call there.

@ned-deily
Copy link
Member

At this point, I think it is up to the 3.12 Release Manager (@Yhg1s) to decide what should be done for 3.12 and whether this is a significant enough risk to ask for Steering Council input. While I think such a change would be positive long term, I would be cautious about making it now without more data on potential breakage, especially with the removal of distutils and the increase in the number of alternate package builders in the intervening years. If there were some way to perhaps query file names in existing 3.11 and 3.12 PyPI packages, the risks could be more confidently assessed?

@AraHaan
Copy link
Contributor

AraHaan commented Apr 9, 2023

Honestly at this point, I can see this as potentially breaking for standalone python (with the stlib in a zip file) for windows as well (which would not be a good idea to break as there are people who do not want to install python because not everyone has administrator privileges (think colleges for example) and so they do the next best thing, use python from a zip file that has everything the interpreter needs all in a single folder). But then again, this change looks to only affect linux so it should be minimally breaking as they do actually install python (also it minimizes the risk that multiple python versions could overwrite the system python's c extensions and break the system itself, the only other issue would be the stlib python scripts though between the system python versions and newer python versions that might use newer syntax that the system python does not know about yet.

@Yhg1s
Copy link
Member

Yhg1s commented Apr 9, 2023

Simply removing the support for untagged extension doesn't sound like a reasonable change. I see no reason why this can't follow the normal deprecation process first. (FWIW, Google uses untagged extension modules all over the place, partly because it doesn't use distutils/setuptools to build them but also because it just works, it's simpler that way, nothing ever complains about it and we have no reason to tag them -- we build everything from source and there's no mixing of Python versions.)

@zooba
Copy link
Member

zooba commented Apr 11, 2023

I don't see why we'd ever want to get rid of it. Certainly we need a longer-than-usual deprecation cycle to change the Windows side (there's currently no supported ABI suffix there for abi3, so extensions using the limited API just use .pyd) as we need to let all the external build tools catch up with whatever change we make. Besides, Windows will already reject incompatible modules on load, so the main motivation from the original email thread doesn't apply.

But honestly, I can't see why this is a big enough issue to need changing anywhere else either. At most, wherever it's documented (is it even documented?) we could warn "hey, here's how this may bite you, and here's the recommended approach". Unless there's a spate of people being caught by this mildly rough edge that I haven't heard about, I'd just drop the whole idea.

@AlexWaygood AlexWaygood changed the title bpo-32387: Remove ability to import non-version-tagged C extensions gh-76568: Remove ability to import non-version-tagged C extensions Apr 11, 2023
@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Apr 15, 2023
@arhadthedev arhadthedev reopened this Apr 26, 2023
@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label May 27, 2023
@brettcannon
Copy link
Member

I closed the issue for this due to lack of enthusiasm for the idea, so I'm closing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO-NOT-MERGE stale Stale PR or inactive for long period of time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.