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

Revised module suitable for new NESTExtensionModule interface #26

Merged
merged 12 commits into from
Apr 26, 2024

Conversation

heplesser
Copy link
Contributor

This is the first example (in draft state) of the new NESTExtensionInterface and ModuleManager approach to handling modules.

@heplesser
Copy link
Contributor Author

Even while the example module still needs a lot of polishing, this one should be reviewed and merged soon so we have one that works with the new NEST mechanism. Then we also need to think about versioning these modules, we need releases that fit the NEST releases.

Copy link
Contributor

@terhorstd terhorstd left a comment

Choose a reason for hiding this comment

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

Good suggestion to switch to mynest namespace.
Some small points

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@terhorstd
Copy link
Contributor

Actions need to be re-run when nest/nest-simulator#3103 is merged.

src/mymodule.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@pnbabu pnbabu left a comment

Choose a reason for hiding this comment

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

I have one minor comment.

Also, what did we decide on the TODOs in the CMakeLists.txt file? Shall we remove them now and create an issue to address it later?

src/mymodule.cpp Outdated Show resolved Hide resolved
@heplesser heplesser removed the request for review from clinssen April 8, 2024 09:45
@pnbabu
Copy link
Contributor

pnbabu commented Apr 15, 2024

@heplesser ping!

@heplesser
Copy link
Contributor Author

@terhorstd @pnbabu I have now addressed your comments, I hope to your content.

Copy link
Contributor

@terhorstd terhorstd left a comment

Choose a reason for hiding this comment

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

Most resolved, two tiny questions remaining.
I think the Python also needs a bump to 3.10 for MacOS to build. I leave it to you if that should be a separate PR and pulled, or if you fix your branch directly.
Thanks!

src/mymodule.cpp Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@pnbabu pnbabu left a comment

Choose a reason for hiding this comment

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

It looks good to me overall. I am fine with merging this after fixing the failing build on macOS.

@heplesser
Copy link
Contributor Author

It looks good to me overall. I am fine with merging this after fixing the failing build on macOS.

I have now hiked the Python version to 3.12, so building should work again on macOS.

@heplesser
Copy link
Contributor Author

But now the Linux build failed with what looks like a problem not related to our code. Strangely, I do not find a re-run button for actions in this repo. Let's see how it goes with the macOS build.

@pnbabu
Copy link
Contributor

pnbabu commented Apr 24, 2024

But now the Linux build failed with what looks like a problem not related to our code. Strangely, I do not find a re-run button for actions in this repo. Let's see how it goes with the macOS build.

Perhaps it's the access issue? I could see the button and the job is running again now.

Copy link
Contributor

@pnbabu pnbabu left a comment

Choose a reason for hiding this comment

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

The CI is green!

Copy link
Contributor

@terhorstd terhorstd left a comment

Choose a reason for hiding this comment

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

At some point we will have a discussion about the Python versions again, but that's a different story and will be told in another PR.
Thanks for all the effort!

@terhorstd terhorstd merged commit 7273612 into nest:master Apr 26, 2024
2 checks passed
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.

3 participants