-
Notifications
You must be signed in to change notification settings - Fork 204
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
Add nanobind #1556
Add nanobind #1556
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI fails due to missing releases.json:
https://github.com/mesonbuild/wrapdb?tab=readme-ov-file#wrap-release-meta
Aside: I would suggest squashing fixup commits (git commit --amend && git push --force
or alternatively, git commit --fixup=HEAD
followed later by git rebase -i --autosquash
).
'src/implicit.cpp', | ||
], | ||
include_directories: [incdir], | ||
install: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nanobind's CMake files have an option whether to install it, do we also want that? If users can choose not to install it, we might want to use:
build_target(...,
target_type: should_install ? 'library' : 'static_library',
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch. If I am reading the CMake configuration correctly it seems like it is only intended to be installed when not used as a subproject - so maybe I should just remove install altogether from this configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Entirely possible. In that case, also change library to static_library, I think...
project( | ||
'nanobind', | ||
'cpp', | ||
version: '2.0.0', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add (for python module including embed:
kwarg support):
meson_version: '>=0.53.0',
1b56955
to
02738e9
Compare
IIRC releases.json has to be in alphabetical order, nanobind is right before nanoarrow but should instead be right after. |
That makes sense. Looks like some of the build failures have to do with the robin-map depdendency version as well - may need to update that as a precursor |
82e4c01
to
e084f83
Compare
@eli-schwartz @WillAyd : If I understand correctly, the goal of this PR is to build using pure Meson without depending on CMake? This would be awesome, but the recipe here looks somehow too easy to me. Please take a look at some of the steps explained in the comment at the top of https://github.com/wjakob/nanobind/blob/master/src/nb_combined.cpp. And this is the "ground truth" reference for the extension build process: https://github.com/wjakob/nanobind/blob/master/cmake/nanobind-config.cmake For example, does this wrapper
Quite a lot of work has gone into the CMake interface produce nice output—I would be concerned about alternatives that produce subpar extensions by missing some of these steps. A good recipe that helped to reproduce the behavior in the Bazel build system (https://github.com/nicholasjng/nanobind-bazel) has been to run nanobind's CMake build sytem on a simple extension in verbose mode and then try to exactly reproduce all of the compiler and linker invocations with their flags on macOS+Windows+Linux (some subtle differences there as well). I'm happy to provide further detail on anything with the caveat that I'll be traveling in July. |
These questions are related. That's something every python module already has to handle. Setuptools handles this automatically, as does maturin. Meson handles it automatically too! CMake doesn't support it, which is why scikit-build and I assume nanobind as well, provide their own cmake modules that define functions which override cmake add_library. Meson calls this
This is a legacy of python 2 which violates the C/C++ language standards. It's not valid code if it has to compile with disabling the mandatory aliasing rules of the C++ language. Since many people get aliasing wrong even though it breaks the language (and so did CPython 2.7!) -- and because sometimes the fact that your code breaks the requirements is difficult to spot -- compiler vendors offer a compiler extension that tells the compiler to cripple its codegen and produce really slow code that tiptoes around aliasing, adding redundant checks here there and everywhere to check at runtime whether the code actually followed the aliasing rules, and recover if it didn't follow the rules. Aliasing violations can't come from CPython since CPython fixed that. They can still come from projects' own code, or from their nanobind dependency. I think you're saying that nanobind needs this extra protection as the
Meson handles this automatically as part of It may make sense to extend the meson language to handle static libraries here somehow. I'll think about this...
This can be added to the
Those are interesting points. The linker response file can probably be handled just like point 5. I'm not sure how to handle point 7 (users could I suppose add that to their own extensions).
Thanks for your insights! |
Does Meson not set most of these in the first place for a release build? I added -DNB_COMPACT_ASSERTIONS but from previous testing the rest were automatically added - so I don't think need to try and manually add here? |
Ignore my previous comment - those flags probably came from the CMake wrap I tried before. Will take another look |
Two quick responses to @eli-schwartz:
Incorrect -- CMake handles it too (via
To my knowledge (and I would be really curious to be pointed to something saying otherwise) Core CPython C code to the cannot be compiled as regular C++ code. It heavily relies on macros that cast and reinterpret data structures that aren't related to each other through inheritance, which violates C++ type punning rules. The |
I'm not sure about this. C of course has its own type punning rules. CPython 2.x did have an infamous problem with it (https://peps.python.org/pep-3123/). I'm not aware of any issue in modern CPython. numpy also has a bunch of C++ which is built without Further, I'm not aware of any construct used in CPython which is legal under C aliasing rules but suddenly illegal under C++ rules. Could you point me to it? |
c604ce2
to
fae150e
Compare
I would use the string method |
Any guidance on how to handle the MSVC errors about a debug build with a release Python interpreter?
Should I just force a release build on msvc? |
Setting the release buildtype in ci_config.json is definitely a reasonable way to deal with this. |
9d536af
to
a57d44f
Compare
The alpine failure is probably because alpine uses -dev packages just like Ubuntu. |
544589c
to
0cac931
Compare
7a4ee59
to
1e8ff6e
Compare
.github/workflows/sanity_checks.yml
Outdated
@@ -47,6 +47,7 @@ jobs: | |||
VisualStudio: | |||
runs-on: windows-latest | |||
strategy: | |||
fail-fast: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temporarily including to make CI verification easier, as pointed out by @benoit-pierre
add_project_link_arguments('-Wl,-dead_strip', '-Wl,x', '-Wl,-S', language: 'cpp') | ||
elif host_machine.system() != 'windows' | ||
add_project_link_arguments('-Wl,-s', language: 'cpp') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't these also be part of dep_link_args?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current CMake configuration sets these as PRIVATE on the target, so just copying that over. Happy to move this to the dependency interface if you think that makes more sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah nevermind - I take that back. I see it is called by both the internal function to build the nanobind library as well as the nanobind_add_module
function that end users are expected to use.
I think what you are proposing makes sense
# nanobind_link_options uses linker response files for either PyPy or | ||
# CPython at this step - how do we support in Meson? | ||
add_project_arguments('-DNB_BUILD', language: 'cpp') | ||
dep_compile_args += ['-DNB_SHARED'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if is_pypy # how to check this?
resp_file = meson.project_source_root() / 'cmake/darwin-ld-pypy.sym'
else
resp_file = meson.project_source_root() / 'cmake/darwin-ld-cpython.sym'
fi
dep_link_args += ['-Wl,@' + resp_file]
@rgommers might remember if we've discussed how to detect pypy.
dep_link_args += ['-Wl,--gc-sections'] | ||
endif | ||
|
||
# Here the CMake configuration sets PIC - does Meson handle this internally? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static_library (and library, to cover static ones) has the pic: <bool>
option. By default it is unset, which means that the value of -Db_staticpic
is used instead, so builders can make this choice. By default, the project option is true: all static libraries will build with PIC.
You can explicitly force this with pic: true
as a kwarg to static_library to prevent anyone from overriding it on the command line. You can also explicitly force it off with pic: false
if you're positive you don't want to build with PIC.
In general, static libraries that may later be used by shared libraries need this, disabling PIC is a niche use case.
if get_option('buildtype') == 'release' | ||
add_project_arguments('-DNDEBUG', '-DNB_COMPACT_ASSERTIONS', language: 'cpp') | ||
endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_option('b_ndebug')
has 3 values: true, false, if-release
Probably, projects should simply specify if-release for their own use. There's no need to manually pass -DNDEBUG IMO.
Opinions vary, apparently. 🤷
Seems like the current VisualStudio x64 failure is reported upstream and a bug with the latest msvc compiler version |
And for the x32 job: #1562. With that get similar errors. |
Thanks @benoit-pierre . My assumption is those two jobs will continue to fail until a newer msvc release comes out (or we can try to test against an older compiler version) |
|
||
py_mod = import('python') | ||
py = py_mod.find_installation() | ||
py_dep = py.dependency(embed: true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
embed: true
is used for cases where you are building an application that contains its own main() function and invokes Py_Initialize().
# Here the CMake configuration links against Python::SABIModule on Windows | ||
# if building an abi3 target - does Meson handle this internally? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a project uses:
py.extension_module(
'foomod',
'foomod.c',
limited_api: '3.12',
)
meson links to python3.dll rather than python312.dll. I assume that's what "SABIModule" is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable. I assume it would be more idiomatic to remove the code that defines Py_LIMITED_API in this configuration and just let Meson handle that
c154a62
to
6d36c85
Compare
1c93107
to
e3800da
Compare
Converted to draft to try out the nanobind commit that works around the MSVC compiler bug and get things green. Probably need to wait until the next nanobind release or MSVC fix, whichever comes first However, I think the configuration itself is ready for any more review comments |
Is the MSVC bug affecting nanobind in CI a blocker for this? This is the upstream report: If not and if there is no further feedback on this, I'd be happy to squash and skip MSVC builds for now to push through |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ready to me...
When using that I notice that I have to do both: meson wrap install robin-map
meson wrap install nanobind Is it expected that the user has to specify this, or did I maybe miss something here that ensures when a user installs nanobind that robin-map gets pulled in as well? |
You aren't missing anything. |
cc @wjakob