Skip to content

Revised module suitable for new NESTExtensionModule interface#26

Merged
terhorstd merged 12 commits intonest:masterfrom
heplesser:modulemgr
Apr 26, 2024
Merged

Revised module suitable for new NESTExtensionModule interface#26
terhorstd merged 12 commits intonest:masterfrom
heplesser:modulemgr

Conversation

@heplesser
Copy link
Copy Markdown
Contributor

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

@heplesser
Copy link
Copy Markdown
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
Copy Markdown
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

Comment thread CMakeLists.txt Outdated
Comment thread CMakeLists.txt Outdated
Comment thread CMakeLists.txt Outdated
Comment thread CMakeLists.txt Outdated
@terhorstd
Copy link
Copy Markdown
Contributor

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

Comment thread src/mymodule.cpp Outdated
Copy link
Copy Markdown
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?

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

pnbabu commented Apr 15, 2024

@heplesser ping!

@heplesser heplesser requested review from pnbabu and terhorstd April 15, 2024 13:45
@heplesser
Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
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!

Comment thread src/mymodule.cpp Outdated
Comment thread CMakeLists.txt Outdated
heplesser and others added 2 commits April 24, 2024 10:21
Co-authored-by: Dennis Terhorst <terhorstd@users.noreply.github.com>
@heplesser heplesser requested a review from terhorstd April 24, 2024 08:23
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
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