Skip to content

Conversation

@ikbuibui
Copy link
Contributor

@ikbuibui ikbuibui commented Dec 2, 2025

The current pattern fails if there are multiple passes of configure and mallocMC was not initially installed.
This happens because in the first pass add_subdirectory creates the package config and in later passes findPackage finds this and fails as mallocMC wasnt actually installed.
Switched to simply using FetchContent with FIND_PACKAGE_ARGS which first checks with findPackage and then falls back to the source dir which is exactly what we want.
https://cmake.org/cmake/help/latest/guide/using-dependencies/index.html#fetchcontent-and-find-package-integration

The current pattern fails if there are multiple passes of configure and
`mallocMC` was not initially installed.
This happens because in the first pass `add_subdirectory` creates the
package config and in later passes `findPackage` finds this and fails as
`mallocMC` wasnt actually installed.
Switched to simply using `FetchContent` with `FIND_PACKAGE_ARGS` which
first checks with `findPackage` and then falls back to the source dir
which is exactly what we want.
https://cmake.org/cmake/help/latest/guide/using-dependencies/index.html#fetchcontent-and-find-package-integration
@ikbuibui ikbuibui added this to the 0.9.0 / next stable milestone Dec 2, 2025
@ikbuibui ikbuibui added component: PMacc in PMacc component: tools scripts, python libs and CMake labels Dec 2, 2025
Copy link
Member

@PrometheusPi PrometheusPi left a comment

Choose a reason for hiding this comment

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

looks good

@chillenzer
Copy link
Contributor

Pertinent infrastructure from mallocMC:

There is an on-going discussion with @psychocoderHPC about pulling up the same approach to PIConGPU (namely add_controlled). I wouldn't let the perfect be the enemy of the good but we should consider and be explicit about how this PR relates to and affects a major overhaul in this regard.

@psychocoderHPC
Copy link
Member

Not sure but I think we can not use a pure fetch content because on clusters without internet we need at least the way to use an installed version of mallocMC. IMO this is not possible via fetch content.
@chillenzer used CMP in mallocMC to allow selecting the behaviour how dependencies will be provided. fetch from web, used pre installed, ...

@ikbuibui
Copy link
Contributor Author

ikbuibui commented Dec 2, 2025

The fetch content here is using 'SOURCE_DIR' without a download method, so it doesnt need internet. It expects mallocMC to be at the pointed directory.
But before this, first it is checking for an installed mallocMC because of the set 'FIND_PACKAGE_ARGS'.

This is exactly the same workflow as before.

@PrometheusPi
Copy link
Member

@psychocoderHPC and @ikbuibui What is your final verdict on the matter of download needed or not?

@ikbuibui
Copy link
Contributor Author

ikbuibui commented Dec 3, 2025

We will further discuss our favoured approaches in the office and then decide on how to proceed.

@ikbuibui
Copy link
Contributor Author

ikbuibui commented Dec 3, 2025

Discussed offline with @psychocoderHPC :
For now this change is okay since it is only fixing a bug while maintaining the status quo, and not changing functionality.
What an ideal solution would be is what @chillenzer was pointing to from his work on mallocMC (add_controlled), which will give the user the option to be explicit on how they want to deal with dependencies.
It is currently implicitly assumed in the cmake code that the we should try to use an installed version first and if that is not available, we will use the subtree. In the future. this implicit decision should be made explicit and provided to the user in a convenient way.

@PrometheusPi PrometheusPi merged commit a054793 into ComputationalRadiationPhysics:dev Dec 3, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: PMacc in PMacc component: tools scripts, python libs and CMake

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants