Conversation
iagaponenko
left a comment
iagaponenko
left a comment
There was a problem hiding this comment.
Looks good to me. I left a couple of suggestions whivh are optional.
| return dir; | ||
| } | ||
|
|
||
| std::string getPackageDirFromAddress(void const* addressInLibrary) { |
There was a problem hiding this comment.
In principle, one could reinforce the code and check that the pointer is not null:
if (addressInLibrary == nullptr) {
throw std::invalid_argument("Null pointer passed the parameter");
}
I'm sure the null pointer will be noticed by dladdr. However, the function doesn't seem to have any specific code for this scenario:
| // The address of a symbol defined in libcpputils resolves, via dladdr, to | ||
| // the cpputils package directory -- independent of any environment variable. | ||
| auto anchor = reinterpret_cast<void const *>(&getPackageDirFromAddress); | ||
| std::filesystem::path cpputilsPath{getPackageDirFromAddress(anchor)}; |
There was a problem hiding this comment.
I would break this line into:
// Test for a valid address
std::filesystem::path cpputilsPath;
BOOST_REQUIRE_NO_THROW({
cpputilsPath = getPackageDirFromAddress(anchor);
});
And if you reinforce the implementation of getPackageDirFromAddress to detect the null pointer, then you may also add this test:
// Now test for the null pointer
BOOST_CHECK_THROW(getPackageDirFromAddress((void*)0), std::invalid_argument);
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new code introduces brittle build/test behavior (missing direct standard includes in packaging.cc and an env-var-dependent assertion in the new test) that should be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
This PR adds an environment-variable-independent way to locate a package root directory by resolving a symbol address to its containing shared library via dladdr, supporting “eups less” installation scenarios.
Changes:
- Add
getPackageDirFromAddress(void const*)API and implementation usingdladdr+std::filesystem. - Add a Boost unit test covering the new lookup behavior.
- Document the new API in the public header.
| File | Description |
|---|---|
include/lsst/cpputils/packaging.h |
Declares and documents the new address-based package-root lookup API. |
src/packaging.cc |
Implements getPackageDirFromAddress using dladdr and filesystem canonicalization. |
tests/test_packaging.cc |
Adds a unit test validating the new lookup and its relationship to the env-var-based lookup. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| #include <dlfcn.h> | ||
|
|
||
| #include <filesystem> | ||
| #include <iostream> | ||
| #include <sstream> |
| // It must agree with the environment-variable-based lookup for the same package. | ||
| BOOST_CHECK_EQUAL(std::filesystem::canonical(cpputilsPath), | ||
| std::filesystem::canonical(getPackageDir("cpputils"))); |
DM-51000: Prepare for eups less installations