-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[core] Generalize TROOT::GetShardLibDir() also on Windows #20810
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
base: master
Are you sure you want to change the base?
Conversation
937a69a to
09d18ef
Compare
core/base/src/TROOT.cxx
Outdated
| DWORD needed = 0; | ||
|
|
||
| HANDLE process = GetCurrentProcess(); | ||
| if (EnumProcessModules(process, modules, sizeof(modules), &needed)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we handle and/or warn about the failure case (eg. if there are more than 1024 libraries or any other error)? Right now, it seems that in the failure case the return value is 'empty'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have now done what is recommended in the Windows API documentation to deal with this: call EnumProcessModules with a larger output array, if the original one was too small.
| for (unsigned int i = 0; i < count; ++i) { | ||
| wchar_t wpath[MAX_PATH]; | ||
| DWORD len = GetModuleFileNameW(modules[i], wpath, MAX_PATH); | ||
| if (!len) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is probably okay to ignore arbitrary library (for which the path is too long) ... but one of them might be libCore resulting in return an empty string. Can we do better and/or can we warn the user in the case libCore was not found?
09d18ef to
65fe1cd
Compare
Test Results 23 files 23 suites 3d 21h 21m 7s ⏱️ Results for commit 71eb847. ♻️ This comment has been updated with latest results. |
09dc626 to
1750224
Compare
This follows up on 47caba5, also generalizing the function on Windows by dynamically finding `libCore.dll` (or `Core.dll` depending on the toolchain). The goal is to avoid using `TROOT::GetRootSys()` in the implementation of this method, with the overarching goal of not having to rely on environment variables like `ROOTSYS` one day, which might not be set in some environments like the Python wheels. A unit test for `TROOT::GetSharedLibDir()` is also implemented.
1750224 to
71eb847
Compare
This follows up on 47caba5, also generalizing the function on Windows by dynamically finding
libCore.dll(orCore.dlldepending on the toolchain).The goal is to avoid using
TROOT::GetRootSys()in the implementation of this method, with the overarching goal of not having to rely on environment variables likeROOTSYSone day, which might not be set in some environments like the Python wheels.I disclose that ChatGPT was used for this mechanical task of translating code that already existed for macOS and Linux to Windows.