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

Remove unused pybind copts #91

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

keith
Copy link
Contributor

@keith keith commented Jun 29, 2024

Since this target has no srcs these are unused

@junyer
Copy link
Contributor

junyer commented Jun 29, 2024

The config_setting is also used in build_defs.bzl, so it would need to be moved there, but... I rather suspect the copts will be used by the parse_headers feature, the --process_headers_in_dependencies flag et cetera? And it seems that they were necessary back in commit 00b4603, so I'm getting real Chesterton's fence vibes. What are your thoughts, @fmeum? :)

(Ping, @rwgk.)

@rwgk
Copy link
Collaborator

rwgk commented Jun 29, 2024

(Ping, @rwgk.)

Ack that I have seen this, but I don't think I know anything that might be helpful here.

"//conditions:default": [
"-fexceptions",
# Useless warnings
"-Xclang-only=-Wno-undefined-inline",
Copy link

Choose a reason for hiding this comment

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

I have never seen this style of flags before. Maybe these are specific to Google's internal complete wrapper and required to get parse_headers to pass with it?

@keith
Copy link
Contributor Author

keith commented Jun 29, 2024

Ah yes parse_headers using these makes sense. I assume @fmeum is right and these flags are specific to Google, since they aren't valid AFAICT. If it's for parse_headers I suppose we could fix them to make that work with bazel's support instead

@junyer
Copy link
Contributor

junyer commented Jun 30, 2024

What if we yeet just the -Xclang-only and -Xgcc-only options – and keep only the -fexceptions option?

Since this target has no srcs these are unused
@keith keith force-pushed the ks/remove-unused-pybind-copts branch from 037ad5d to 564cc2b Compare July 1, 2024 23:51
@keith
Copy link
Contributor Author

keith commented Jul 1, 2024

sounds good, updated to that. Looks like using parse_headers fails in python headers either way right now (at least on macOS):

% bazel build ... --process_headers_in_dependencies --features=parse_headers
...

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
external/rules_python~~python~python_3_11_aarch64-apple-darwin/include/python3.11/bytesobject.h:27:1: error: unknown type name 'PyAPI_DATA'
PyAPI_DATA(PyTypeObject) PyBytes_Type;
^
external/rules_python~~python~python_3_11_aarch64-apple-darwin/include/python3.11/bytesobject.h:27:25: error: expected ';' after top level declarator
PyAPI_DATA(PyTypeObject) PyBytes_Type;
                        ^
                        ;
external/rules_python~~python~python_3_11_aarch64-apple-darwin/include/python3.11/bytesobject.h:28:1: error: unknown type name 'PyAPI_DATA'
PyAPI_DATA(PyTypeObject) PyBytesIter_Type;
^

@keith
Copy link
Contributor Author

keith commented Jul 23, 2024

I think this one is good to go

@jiawen
Copy link
Collaborator

jiawen commented Aug 6, 2024

I think this one is good to go

I'll take a look at this when I get a few cycles (i.e., tomorrow) but might need some time to grok the issue.

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.

5 participants