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

Rename conly to enable_cxx and flip default behavior #44

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jsharpe
Copy link
Contributor

@jsharpe jsharpe commented Oct 26, 2021

This is a suggestion that the default of using C++ configuration is the wrong choice. Most python extensions are written in C and the behaviour of passing a c++ only flag e.g. -std=c++17 to a compiler in C mode is generally tolerated but not by all (e.g. clang 13 specifies this as an error).

Surely it is better for the few packages that use C++ to require explicitly opting in to C++ flags? I found that flipping this default didn't break the build of any of the pip packages I am importing.

@jhance
Copy link
Contributor

jhance commented Oct 27, 2021

I imagine that cases where we need to know that this flag even exists (which I did not know) are much less with the current default?

I would need to check how intrusive this is locally. The logic that it breaks clang makes sense especially if our conly flags are wrong for the pip packages in this repo.

@jhance
Copy link
Contributor

jhance commented Oct 27, 2021

Okay, we agree that this default makes more sense. But I think the name conly does not make sense if it defaults to True. Instead, lets rename to enable_cxx and default it to False. If you do this I will merge this.

@jsharpe
Copy link
Contributor Author

jsharpe commented Oct 27, 2021

@jhance I've updated it to use enable_cxx however I've left the cffi interface using conly as due to the use of kwargs this would probably silently break consumers of this macro. Feel free to change this if you prefer to switch this use too.

@jhance
Copy link
Contributor

jhance commented Oct 27, 2021

I don't agree with your diagnosis of cffi silently breaking due to kwargs. Its passing the kwargs to something that doesn't accept the conly parameter as far as I can tell, so things passing conly=True would still error. That said, easy change to make on my end when I import this.

This will take me longer to import since it is a breaking change for us internally.

@jhance jhance changed the title Default to conly for pip built libraries Rename conly to enable_cxx and flip default behavior Oct 27, 2021
@jsharpe
Copy link
Contributor Author

jsharpe commented Oct 27, 2021

Ok, I didn't follow through the logic of where kwargs was being used.

@jsharpe
Copy link
Contributor Author

jsharpe commented Mar 1, 2022

@jhance any updates on this PR?

@CLAassistant
Copy link

CLAassistant commented Apr 16, 2022

CLA assistant check
All committers have signed the CLA.

@jsharpe
Copy link
Contributor Author

jsharpe commented Aug 16, 2022

Ping @jhance

@jsharpe
Copy link
Contributor Author

jsharpe commented Oct 31, 2022

@jhance did you ever manage to land this change internally?

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

Successfully merging this pull request may close these issues.

3 participants