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

Modularize libmambapy #2960

Merged
merged 13 commits into from
Nov 10, 2023
Merged

Conversation

AntoinePrv
Copy link
Member

  • Modularize libmambapy
  • Add test-libmambapy to Taskfile

@AntoinePrv AntoinePrv changed the title Mpdularize libmambapy Modularize libmambapy Nov 6, 2023
@AntoinePrv AntoinePrv force-pushed the libmambapy-modular branch 2 times, most recently from 7d47f84 to e47b119 Compare November 7, 2023 16:38
@AntoinePrv AntoinePrv marked this pull request as ready for review November 7, 2023 18:12
Copy link
Member

@JohanMabille JohanMabille left a comment

Choose a reason for hiding this comment

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

I find it confusing to have the file main.cpp in the core subfolder (implying it's not the main for the whole library), defining the bindings, and having the main macro PYDBIND11_MODULE called in the file core.cpp file. It looks to me like the logic is inverted.

Even if we split the bindings in many files (which I agree is far better than a huge monolithic file), we will keep a single global module (libmambapy), and the name passed to the PYBIND11_MODULE macro should be the same as that of the extension library (dll/dylib/so) to avoid this issue (although the tests seem to pass, so I might have missed something).

A more natural code organization would be one pair of hpp/cpp per libmamba subfolder that we want to bind, and the main.cpp defining the module (i.e. calling PYBIND11_MODULE) and calling the bind_submodule functions defined in the other files.

@AntoinePrv AntoinePrv merged commit 0d42e81 into mamba-org:main Nov 10, 2023
27 checks passed
@AntoinePrv AntoinePrv deleted the libmambapy-modular branch November 10, 2023 17:29
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.

None yet

2 participants