Skip to content

bpo-33466: Support compiling Objective-C++ (“.mm”) files #6767

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 2 commits into from
Closed

bpo-33466: Support compiling Objective-C++ (“.mm”) files #6767

wants to merge 2 commits into from

Conversation

fish2000
Copy link
Contributor

@fish2000 fish2000 commented May 11, 2018

At the moment, Objective-C++ files suffixed with .mm are unrecognized; however, other parts of distutils suggest support for compiling these files – e.g. the suffix list found in extension.py:

if suffix in (".c", ".cc", ".cpp", ".cxx", ".c++", ".m", ".mm"):

Adding the ".mm" string to the src_extensions list fixes this inequity.

https://bugs.python.org/issue33466

At the moment, Objective-C++ files suffixed with `.mm` are unrecognized; however, other parts of `distutils` suggest support for compiling these files (e.g. the suffix list found in https://github.com/python/cpython/blob/7ec8f28656ea9d84048e9b5655375c6a74a59f53/Lib/distutils/extension.py#L193). Adding the `".mm"` string to the `src_extensions` list fixes this inequity.
@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

When your account is ready, please add a comment in this pull request
and a Python core developer will remove the CLA not signed label
to make the bot check again.

Thanks again to your contribution and we look forward to looking at it!

@fish2000
Copy link
Contributor Author

@fish2000
Copy link
Contributor Author

It looks like b.p.o. has acknowledged my signature of the CLA:
screen shot 2018-05-14 at 12 54 09 pm

@ronaldoussoren
Copy link
Contributor

Patch looks good to me.

P.S. I'm not merging at this time because this would the first patch I'd merge in a long time and I need to check the devguide to ensure I follow the right process.

Copy link
Member

@merwok merwok left a comment

Choose a reason for hiding this comment

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

Hello! Could you edit the docs to mention this new feature?

@bedevere-bot
Copy link

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. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@csabella
Copy link
Contributor

@fish2000, it looks like your pull request was approved by a core developer and had some changes requested by another. If you're able to address the code review, perhaps this can be merged soon. Thanks!

@merwok
Copy link
Member

merwok commented Oct 20, 2020

@ronaldoussoren would you like to pronounce on this?

This looks innocuous and could be merged after a manual test (to confirm that it works) and a quick doc addition. On the other hand, distutils is strongly deprecated and apparently on its way out, people have been advised to use setuptools or other build tools for years, so maybe distutils PRs should be closed and patches redirected to setuptools.

Copy link
Contributor

@ronaldoussoren ronaldoussoren left a comment

Choose a reason for hiding this comment

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

LTGM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants