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

Update cppyy #60

Closed
wants to merge 2 commits into from
Closed

Update cppyy #60

wants to merge 2 commits into from

Conversation

keceli
Copy link
Contributor

@keceli keceli commented Dec 22, 2022

TODO:
.cmake files are not synced anymore, see issue #48. Should we enable sync or move them to a new repo or to CmakePP?

@ryanmrichard
Copy link
Member

On second thought, I guess I didn't think the placement of these files through well. I think the easiest solution is to make a new repo NWXCMake (or something) which holds project-wide CMake modules. Then we can just fetchcontent that repo from other repos in the organization.

@keceli
Copy link
Contributor Author

keceli commented Dec 22, 2022

On second thought, I guess I didn't think the placement of these files through well. I think the easiest solution is to make a new repo NWXCMake (or something) which holds project-wide CMake modules. Then we can just fetchcontent that repo from other repos in the organization.

I am not a fan of adding a new repo in general and in this case it requires changes in each repo.

I'd prefer/vote for enabling sync. There might be other files (gitignore, code of conduct, contributing etc) that we would like to sync as well.

@keceli
Copy link
Contributor Author

keceli commented Dec 22, 2022

Or if possible, moving cmake modules to CMakePP.

@ryanmrichard
Copy link
Member

We moved away from synch (at least the action we used before) because it didn't work well. GitHub seems to be very against repos triggering actions in other repos (it makes sense because you can imagine how that could turn into more or less a DDOS attack). In turn, it felt like we were always fighting GitHub to keep it running. Then there was the fact that quite often we had to manually trigger pushes to get it to work; I never did figure out what caused this need, but it very well could be related to the former point. At this point, I feel that needing to copy/paste (even if it's automated copy/paste) things is an anti-pattern and I'd like to avoid it.

CMakePP is intended to be a stand alone project, so I'm hesitant to put CMake modules there unless they are very general. The ones throughout NWX are pretty specific to our needs.

I guess I'm fine leaving the CMake modules in this repo. If we fetch-content this repo we'll get some extra stuff, but I don't think it's so much extra stuff to really impact performance.

@ryanmrichard
Copy link
Member

Anyone else want to weigh in before we merge this?

@ryanmrichard
Copy link
Member

@keceli was this superseded by creating the NWXCMake repo?

@keceli
Copy link
Contributor Author

keceli commented Mar 17, 2023

@keceli was this superseded by creating the NWXCMake repo?

Yes, should have closed it earlier.

@keceli keceli closed this Mar 17, 2023
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.

Build should crash if Cppyy is not found (and Python Bindings are enabled)
2 participants