Skip to content

change the include style in _pymodule.h and remove unused or duplicate headers in two header files - #8473

Merged
sklam merged 1 commit into
numba:mainfrom
dlee992:optimize_include_style_2
Jan 25, 2023
Merged

change the include style in _pymodule.h and remove unused or duplicate headers in two header files#8473
sklam merged 1 commit into
numba:mainfrom
dlee992:optimize_include_style_2

Conversation

@dlee992

@dlee992 dlee992 commented Sep 28, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Hi, guys.

In this PR, I changed the include style from <..> to ".." in _pymodule.h. Since I want to use some Numba headers in my numba extension, and meanwhile I use bazel to build a cc file in my extension (because I also depends on a 3rd C++ library). But because of these "ill-formed" include usages, bazel will output some errors to me, like this:

external/numba/cext/../_pymodule.h:6:10: error: 'Python.h' file not found with <angled> include; use "quotes" instead

Meanwhile, I also deleted Python.h in listobject.h and dictobject.h, because cext.h will include this for them. I also deleted ../_pymodule.h in dictobject.h, since they seem not use any marcos or other things in it.

After these changes, the building process seems still good.

PS, I hope these changes in _pymodule.h can be merged into a recent Numba release version at least, thanks.

=============
updates [Sept. 29]: After changing the include style in the _pymodule.h, bazel can successfully compile the cc file for me.

Sure, I have done some tricky things in WORKSPACE:

# Convert the python include dir to a local repo
new_local_repository(
    name = "python",
    # FIXME: DON'T use hardcode python include path
    path = "/Users/ld/opt/anaconda3/envs/numba-pandas/include/python3.10",
    build_file_content = """
package(default_visibility = ["//visibility:public"])
cc_library(
    name = "headers",
    hdrs = glob(["**/*.h"])
)
"""
)

# Convert the site-packages/numba dir to a local repo
new_local_repository(
    name = "numba",
    path = "/Users/ld/opt/anaconda3/envs/numba-pandas/lib/python3.10/site-packages/numba",
    build_file_content = """
package(default_visibility = ["//visibility:public"])
cc_library(
    name = "headers",
    hdrs = glob(["**/*.h"])
)
"""
)

Then, add these two headers into the cc_binary arguments.

gmarkall
gmarkall approved these changes Oct 4, 2022
View reviewed changes

@gmarkall gmarkall left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good to me - I think that the change is a move in the right direction - my understanding is that in general the search order of both < / > and " / " includes is implementation-defined, but in general one ought to only expect system paths to be searched when using angle brackets. For example from the GCC Search Path docs:

For the angle-bracket form #include <file>, the preprocessor’s default behavior is to look only in the standard system directories.

We have been getting away with this because the distutils / setuptools / Python build setup (I'm not sure exactly which / what) adds the Python include dir as a system dir - for example, I see -isystem /home/gmarkall/mambaforge/envs/numbadev/include on the compiler's command line when building Numba. I am actually a little surprised that this never caused an issue before :-)

I do think this should have a full buildfarm run to ensure it doesn't cause a surprise build failure on any of the platforms we support, but other than that I have confidence in this change.

@gmarkall gmarkall added Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm 4 - Waiting on CI Review etc done, waiting for CI to finish and removed 3 - Ready for Review labels Oct 4, 2022
@gmarkall

gmarkall commented Oct 4, 2022

Copy link
Copy Markdown
Member

@esc / @stuartarchibald Could this have a buildfarm run please?

@stuartarchibald

stuartarchibald commented Oct 4, 2022

Copy link
Copy Markdown
Contributor

Thanks for writing up our OOB discussion @gmarkall. The cext.h header already uses #include "Python.h" and as a result the explicit inclusion of this in the typed container headers is indeed superfluous. Changing the search path also makes sense for the reasons outlined by @gmarkall.

Thanks for the patch @dlee992 !

@gmarkall

gmarkall commented Jan 11, 2023

Copy link
Copy Markdown
Member

@sklam / @stuartarchibald Can this have a buildfarm run please? I think it's the only thing it's waiting on since we decided this change was OK, but want to check there's no surprises on all platforms.

@sklam

sklam commented Jan 17, 2023

Copy link
Copy Markdown
Member

BFID: numba_smoketest_cpu_yaml_158

@stuartarchibald

stuartarchibald commented Jan 25, 2023

Copy link
Copy Markdown
Contributor

BFID: numba_smoketest_cpu_yaml_158

This passed with the exception of a known failure on osx-arm64 due to #8567.

@gmarkall gmarkall added BuildFarm Passed For PRs that have been through the buildfarm and passed 5 - Ready to merge Review and testing done, is ready to merge and removed Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm 4 - Waiting on CI Review etc done, waiting for CI to finish labels Jan 25, 2023
@sklam sklam added this to the Numba 0.57 RC milestone Jan 25, 2023
@sklam
sklam merged commit 276a8d1 into numba:main Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gmarkall gmarkall gmarkall approved these changes
@sklam sklam Awaiting requested review from sklam
@stuartarchibald stuartarchibald Awaiting requested review from stuartarchibald

Assignees

@gmarkall gmarkall

@stuartarchibald stuartarchibald

Labels

5 - Ready to merge Review and testing done, is ready to merge BuildFarm Passed For PRs that have been through the buildfarm and passed

Projects

None yet

Milestone

Numba 0.57 RC

Development

Successfully merging this pull request may close these issues.

4 participants

@dlee992 @gmarkall @stuartarchibald @sklam