Skip to content

Conversation

@abachma2
Copy link
Member

This PR updates the CMakeLists.txt file to require at least C++ 14so that the project can be built. The project builds and the unit tests in recycle_unit_tests executable all pass locally.

@abachma2 abachma2 requested review from bam241, gonuke and nsryan2 July 17, 2025 19:26
@abachma2 abachma2 self-assigned this Jul 17, 2025
@abachma2 abachma2 added the bug label Jul 17, 2025
@gonuke
Copy link
Member

gonuke commented Jul 18, 2025

There were some major overhauls to Cycamore's CMake infrastructure in the 2nd half of last year that should simplify things if they were to be transferred to here

@abachma2
Copy link
Member Author

My first thought for this update was to copy over what is in Cycamore and then just update the library name. That ended up causing some issues because the src directory here has a sub-directory (toolkit) that has a subdirectory (pyroprocessing) and Cycamore does not have nay subdirectories in src. So some of the build doesn't get linked properly with the contents of the Cycamore CMakeLists and I'm not versed enough in CMake to more selectively pull from the Cycamore file.

@gonuke
Copy link
Member

gonuke commented Jul 18, 2025

My first thought for this update was to copy over what is in Cycamore and then just update the library name. That ended up causing some issues because the src directory here has a sub-directory (toolkit) that has a subdirectory (pyroprocessing) and Cycamore does not have nay subdirectories in src. So some of the build doesn't get linked properly with the contents of the Cycamore CMakeLists and I'm not versed enough in CMake to more selectively pull from the Cycamore file.

Hmmm.... this is definitely not a conventional design.... some thought will be required.

@abachma2
Copy link
Member Author

Besides some things in the repo structure being unconventional and worth re-visiting, is there anything that should be updated with this PR?

@gonuke
Copy link
Member

gonuke commented Jul 29, 2025

Does it work? I know that there was substantial restructuring of CMake and its macros, so I'm not sure if that kind of restructuring makes sense here, just on the CMake side, never mind the code side.

@gonuke
Copy link
Member

gonuke commented Jul 29, 2025

We might be better off, after all, starting in a clean repo for advanced archetypes, and then moving Pyre into that repo...???

@abachma2
Copy link
Member Author

It builds on my machine (rocky 8.9). When I get around to updating the CI I can run on other machine types.

I'm not against moving this to a new repo, but the benefit of that is not clear to me.

@gonuke
Copy link
Member

gonuke commented Aug 6, 2025

It builds on my machine (rocky 8.9). When I get around to updating the CI I can run on other machine types.

I'm not against moving this to a new repo, but the benefit of that is not clear to me.

Let's leave things here. It is probably worth taking a look at the current version of Cycamore's CMakeLists.txt file and making a few additional changes here. Most/all of them are simplifying changes that come from moving some standard things into macros that can now be imported by all archetype developers.

Alternatively, we could make that a separate issue and just go forward with this now that it builds.

@gonuke
Copy link
Member

gonuke commented Aug 11, 2025

Alternatively, we could make that a separate issue and just go forward with this now that it builds

I made issue #26 to capture this.

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

I wonder if we need to update CI first so we have tests that show this building. Or maybe they need to be part of the same PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants