-
Notifications
You must be signed in to change notification settings - Fork 763
{lib}[GCCcore/14.3.0] spdlog-1.15.3 #24391
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
{lib}[GCCcore/14.3.0] spdlog-1.15.3 #24391
Conversation
|
The build of shared libraries was added in #24310, but enabling the tests is probably a good idea. We shouldn't remove building with PIC though, as this can cause issues when using the library later on. |
Ah, that PR has slipped through when I was working on that. I agree with the PIC, I will update that. |
|
@boegelbot please test @ jsc-zen3 |
|
@sassy-crick: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de PR test command '
Test results coming soon (I hope)... - notification for comment with ID 3460840222 processed Message to humans: this is just bookkeeping information for me, |
|
Test report by @boegelbot |
|
@boegelbot please test @ jsc-zen3 |
|
@sassy-crick: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de PR test command '
Test results coming soon (I hope)... - notification for comment with ID 3460993457 processed Message to humans: this is just bookkeeping information for me, |
|
Test report by @boegelbot |
|
Test report by @Thyre |
|
Test report by @Thyre |
|
Test report by @Crivella |
| _shared_configopts = " ".join([ | ||
| "-DSPDLOG_BUILD_SHARED=ON", | ||
| "-DSPDLOG_BUILD_PIC=ON", | ||
| ]) | ||
| _base_configopts = ' -DSPDLOG_BUILD_TESTS=ON' | ||
| configopts = [f'{_base_configopts} -DSPDLOG_BUILD_SHARED={x}' for x in ('OFF', 'ON')] |
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.
Not sure if we want to remove SPDLOG_BUILD_PIC=ON
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 doing what we are already doing in the CMake EB when setting the fpic TC option to true
But i would still leave it there to ensure that we follow what the source CMake dictates in case they add more stuff in the future
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 was not aware of the other PR so mine was based on a different 'template' and thus, I did not remove it, I simply never added it. I have put that back, thanks for finding this!
| sanity_check_paths = { | ||
| 'files': ['include/%(name)s/%(name)s.h', 'lib/lib%(name)s.a', f'lib/lib%(name)s.{SHLIB_EXT}'], | ||
| 'files': [f'include/{name}/{name}.h', 'lib/libspdlog.a', 'lib/libspdlog.%s' % SHLIB_EXT], | ||
| 'dirs': ['lib64/cmake', 'lib64/pkgconfig'], |
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.
Would keep consistent usage of f-strings without re-introducing old % formatting.
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.
Well, I done my PR without being aware of what you already done @Crivella . So that is why I had already the f-string for the bit I knew how to do it, but I was not sure how to do it for the SHLIB_EXT bit. So now that is consistent, thanks fo your help.
|
|
||
| sanity_check_paths = { | ||
| 'files': ['include/%(name)s/%(name)s.h', 'lib/lib%(name)s.a', f'lib/lib%(name)s.{SHLIB_EXT}'], | ||
| 'files': [f'include/{name}/{name}.h', 'lib/libspdlog.a', 'lib/libspdlog.%s' % SHLIB_EXT], |
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.
| 'files': [f'include/{name}/{name}.h', 'lib/libspdlog.a', 'lib/libspdlog.%s' % SHLIB_EXT], | |
| 'files': ['include/%(name)s/%(name)s.h', 'lib/lib%(name)s.a', f'lib/lib%(name)s.{SHLIB_EXT}'], |
%(name)sare EB templates. I do not expect it to ever be different fromnamein the EC but i would keep it as such w.r.t other ECsSHLIB_EXTis also an EB variable that will be injected in the EC. with python linting it appears undefined but it will be replaced when the EC is loaded
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.
My understanding was the %(foo)s is better replaced with f'{foo}' as it is easier to read. Did I get that wrong?
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.
They are not equivalent. What i was referring to is the % used to place SHLIB_EXT in the string.
%(name)s is not a string formatting but an EB template that is replaced by EB when reading the EasyConfig.
The ones under https://docs.easybuild.io/version-specific/easyconfig-templates/#template-namesvalues-as-set-in-easyconfig are probably the only trivial ones that will be equivalent to the value in the EC
EDIT: for context we already have some {name} in EB but templates are the majority
(easybuild-dev) crivella@crivella-desktop:~/Documents/GIT/easybuild-easyconfigs $ grep -l -R '{name}' * | wc -l
124
(easybuild-dev) crivella@crivella-desktop:~/Documents/GIT/easybuild-easyconfigs $ grep -l -R '%(name)s' * | wc -l
2249
|
Test report by @Crivella |
Crivella
left a comment
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.
LGTM
|
Going in, thanks @sassy-crick! |
…ial missing PIC in static library
`spdlog` apply changes from #24391 to all versions.
(created using
eb --new-pr)Added the build of the dynamic libraries which are needed for some software packages, and added the one test job they provide.
Happy to backport to the already existing EC if required.