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

Expose a subset of distutils C-compiler interface as public API #3445

Closed
wants to merge 5 commits into from

Conversation

neutrinoceros
Copy link

@neutrinoceros neutrinoceros commented Jul 13, 2022

Summary of changes

Re-expose a subset of distutils C-compiler interface as setuptools.ccompiler, as suggested by @abravalheri.
I went with the most conservative approach and exposed only the components I know are needed downstream.
I'll happily add more of them on request (ping @minrk), or expose the whole distutils.ccompiler module if prefered by maintainers.

Closes #2806

I'm not sure what kind of tests this would require. Would it suffice to check that these names are defined ?

Pull Request Checklist

  • Changes have tests
  • News fragment added in [changelog.d/].
    (See [documentation][PR docs] for details)

@abravalheri
Copy link
Contributor

I'm not sure what kind of tests this would require. Would it suffice to check that these names are defined ?

The tests from _distutils do not currently run in the setuptools CI, so I think it would make sense to add some type of tests.

It can be anything very simple, as long as it exercises the required APIs, just to make sure there is no regression in the future.

@minrk
Copy link
Contributor

minrk commented Jul 13, 2022

I believe that works for me (pyzmq). Before I refactored things in zeromq/pyzmq#1632, I also called get_default_compiler() which returns the default compiler type, but I'm not sure this was truly necessary. The same answer can be accessed with new_compiler().compiler_type.

@neutrinoceros
Copy link
Author

neutrinoceros commented Jul 13, 2022

@minrk turns out I think I don't actually need to re-export the CCompiler class for our project. Do you need it or can I remove it ?
edit: I was wrong, we do need it

@neutrinoceros
Copy link
Author

thanks @abravalheri, I've added a first minimal test, let me know if you think anything more is needed

@minrk
Copy link
Contributor

minrk commented Jul 13, 2022

I don't need CCompiler, really just new_compiler and customize_compiler.

@neutrinoceros
Copy link
Author

Ok thank you both for your quick responses. Failures look unrelated (at least to my untrained eye), so let's updraft this

@neutrinoceros neutrinoceros marked this pull request as ready for review July 13, 2022 18:19
@abravalheri
Copy link
Contributor

Thank you very much @neutrinoceros. I will prefer to leave the review/final decision on this one to Jason, maybe he has something different in mind.

@abravalheri abravalheri requested a review from jaraco July 13, 2022 18:25
@neutrinoceros
Copy link
Author

Failures look unrelated (at least to my untrained eye)

Seeing that CI is working correctly for all other PRs I'll take that back.
I was able to fix my own test, but the remaining issues are hard to decipher for me, any help would be appreciated

Copy link
Contributor

@abravalheri abravalheri left a comment

Choose a reason for hiding this comment

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

Seeing that CI is working correctly for all other PRs I'll take that back.
I was able to fix my own test, but the remaining issues are hard to decipher for me, any help would be appreciated

I did some tests here and it would seem that just importing customize_compiler changes some global state. Any other test following test_ccompiler that depends on a different aspect of the global state may break.

I believe that we can counteract this with 2 measures:

  1. Be lazy about importing disutils.sysconfig
  2. Try to force distutils.sysconfig to be executed again if any of the following tests depends on it.

I added some code changes suggestions with this in mind, but just as a starting point. Please feel free to further iterate on them, or completely ignore the suggestions.

setuptools/ccompiler.py Outdated Show resolved Hide resolved
setuptools/tests/test_ccompiler.py Show resolved Hide resolved
setuptools/tests/test_ccompiler.py Outdated Show resolved Hide resolved
setuptools/tests/test_ccompiler.py Show resolved Hide resolved
neutrinoceros and others added 2 commits July 19, 2022 17:19
Co-authored-by: Anderson Bravalheri <[email protected]>
Co-authored-by: Anderson Bravalheri <[email protected]>
@abravalheri
Copy link
Contributor

Ok, so now we only see 2 flake8 errors.

I suppose you can get rid of them with __all__ = ["CCompiler", "customize_compiler", "new_compiler"].

@jaraco
Copy link
Member

jaraco commented Jul 23, 2022

Thanks. I appreciate all the work here, but I'm going to suggest we not simply expose these names until we understand better the use-cases driving these names. Currently, the names are exposed under distutils.ccompiler, which is the compatibility shim. I'd suggest to continue to use that until we can determine what is the best design going forward.

@neutrinoceros
Copy link
Author

neutrinoceros commented Jul 23, 2022

AFAIC you can close this if there's nothing you wish to keep from it. I don't mind that this solution isn't retained, and will happily settle for any other :)

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.

Expose compiler classes for customized use
4 participants