-
Notifications
You must be signed in to change notification settings - Fork 62
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
ci: Enable tests for ROOT5 GCC11 environment #370
Conversation
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.
As discussed these changes are fine with me and do not cause issues.
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 don't have any issues with the changes, save for one. Add the line using std::abs
in AgMath.h. This will ensure that a floating point version of the abs function is used, rather than rely on whatever version gets included through TMath.h. (Worst case, it uses the cstdlib version, which will happily cast to integer and break geometries.)
As per request by @klendathu2k at star-bnl#370 (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.
Looks good...
Sure, let's consider other macros. Do you have anything specific in mind? |
On Jun 29, 2022, at 5:08 PM, Dmitri Smirnov ***@***.******@***.***>> wrote:
Sure, let's consider other macros. Do you have anything specific in mind?
grep StEventUtilities $STAR/StRoot/macros/*.C $STAR/StRoot/macros/*/*.C
/afs/rhic.bnl.gov/star/packages/SL21a/StRoot/macros/Load.C<http://rhic.bnl.gov/star/packages/SL21a/StRoot/macros/Load.C>:void Load(Char_t *loadList="St_base,St_Tables,StChain,StDetectorDbMaker,StBichsel,StEvent,StTpcDb,StUtilities,StDbLib,StDbBroker,St_db_Maker,StTriggerDataMaker,StEventUtilities,StBFChain"){
/afs/rhic.bnl.gov/star/packages/SL21a/StRoot/macros/loadMuDst.C<http://rhic.bnl.gov/star/packages/SL21a/StRoot/macros/loadMuDst.C>: gSystem->Load("StEventUtilities");
/afs/rhic.bnl.gov/star/packages/SL21a/StRoot/macros/makePicoDst.C<http://rhic.bnl.gov/star/packages/SL21a/StRoot/macros/makePicoDst.C>: gSystem->Load("StEventUtilities");
/afs/rhic.bnl.gov/star/packages/SL21a/StRoot/macros/RedoSpaceCharge.C<http://rhic.bnl.gov/star/packages/SL21a/StRoot/macros/RedoSpaceCharge.C>: gSystem->Load("StEventUtilities");
/afs/rhic.bnl.gov/star/packages/SL21a/StRoot/macros/StMuDstMakerYear1.C<http://rhic.bnl.gov/star/packages/SL21a/StRoot/macros/StMuDstMakerYear1.C>: gSystem->Load("StEventUtilities");
/afs/rhic.bnl.gov/star/packages/SL21a/StRoot/macros/analysis/doEvents.C<http://rhic.bnl.gov/star/packages/SL21a/StRoot/macros/analysis/doEvents.C>: gSystem->Load("StEventUtilities");
/afs/rhic.bnl.gov/star/packages/SL21a/StRoot/macros/analysis/doEvents.C<http://rhic.bnl.gov/star/packages/SL21a/StRoot/macros/analysis/doEvents.C>:// Load StEventUtilities
/afs/rhic.bnl.gov/star/packages/SL21a/StRoot/macros/analysis/find_vertex.C<http://rhic.bnl.gov/star/packages/SL21a/StRoot/macros/analysis/find_vertex.C>: gSystem->Load("StEventUtilities");
/afs/rhic.bnl.gov/star/packages/SL21a/StRoot/macros/graphics/Draw3DDoc.C<http://rhic.bnl.gov/star/packages/SL21a/StRoot/macros/graphics/Draw3DDoc.C>: doc.SetSourceDir(".:src:inc:StRoot:StRoot/StarRoot:StRoot/StEventUtilitie");
/afs/rhic.bnl.gov/star/packages/SL21a/StRoot/macros/graphics/GeomBrowse.C<http://rhic.bnl.gov/star/packages/SL21a/StRoot/macros/graphics/GeomBrowse.C>: gSystem->Load("StEventUtilities");
… |
Gotta love github's automatic formatting. Trying again from the gui:
|
Have you considered all of them when you removed the dependency on Stu? |
I considered them when I added the dependency in BFC, and that drove me to avoid adding the dependency if it wasn't necessary. Thus I removed it shortly after adding it.
…-Gene
On Jun 29, 2022, at 5:37 PM, Dmitri Smirnov ***@***.******@***.***>> wrote:
Have you considered all of them when you removed the dependency on Stu?
|
Did you run all of these macros with default parameters? |
Nope.
… On Jun 29, 2022, at 9:07 PM, Dmitri Smirnov ***@***.***> wrote:
Did you run these macros with default parameters?
|
Would you then care to share how you run the tests? |
On Jun 29, 2022, at 10:48 PM, Dmitri Smirnov ***@***.***> wrote:
Would you then care to share how you run the tests?
I did not say I ran tests. But I think you've determined that StTpcDb.so needs to be loaded before StEventUtilities.so. I expect that a consequence of this is that all macros which load StEventUtilities.so must load StTpcDb.so beforehand. If those macros are not relevant for, not impacted by this PR, then you should just ignore my question about considering these macros.
…-Gene
|
I see the following files modified in 79e3801 "Remove StTpcDb dependence for StEventUtilities"
UPDATE. The error from
|
In my previous comment I forgot to mention that running
St_geom_Maker does not exist since 2019: 173d6b4 - (3 years, 3 months ago) Moved to the attic - Jerome Lauret
So, Gene, I am not sure where we want to go from here. Revert the entire 79e3801 "Remove StTpcDb dependence for StEventUtilities" ? |
If a macro requires retired code, I would consider the macro retired and remove it. |
The compiler errors are: ``` g++ -fPIC -pipe -Wall -Woverloaded-virtual -std=c++0x -Wno-long-long -pthread -Wno-deprecated-declarations -m64 -O2 -g -falign-loops -falign-jumps -falign-functions -Dsl79_gcc11 -D__ROOT__ -DNEW_DAQ_READER -I. -IStRoot -I.sl79_gcc11/include -I/opt/software/linux-scientific7-x86_64/gcc-11.2.1/root-5.34.38-p7sjrh5whm6m2ao3zxkaoaqomiscvw52/include -c .sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgAttribute.cxx -o .sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgAttribute.o In file included from .sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgBlock.h:17, from .sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgAttribute.cxx:2: .sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgMath.h:57:28: error: 'double abs(double)' conflicts with a previous declaration 57 | inline double abs (double x) {return fabs(x);} | ^ In file included from /opt/rh/devtoolset-11/root/usr/include/c++/11/cstdlib:77, from /opt/rh/devtoolset-11/root/usr/include/c++/11/ext/string_conversions.h:41, from /opt/rh/devtoolset-11/root/usr/include/c++/11/bits/basic_string.h:6607, from /opt/rh/devtoolset-11/root/usr/include/c++/11/string:55, from /opt/software/linux-scientific7-x86_64/gcc-11.2.1/root-5.34.38-p7sjrh5whm6m2ao3zxkaoaqomiscvw52/include/TSchemaHelper.h:25, from /opt/software/linux-scientific7-x86_64/gcc-11.2.1/root-5.34.38-p7sjrh5whm6m2ao3zxkaoaqomiscvw52/include/TGenericClassInfo.h:17, from /opt/software/linux-scientific7-x86_64/gcc-11.2.1/root-5.34.38-p7sjrh5whm6m2ao3zxkaoaqomiscvw52/include/Rtypes.h:270, from /opt/software/linux-scientific7-x86_64/gcc-11.2.1/root-5.34.38-p7sjrh5whm6m2ao3zxkaoaqomiscvw52/include/TObject.h:31, from /opt/software/linux-scientific7-x86_64/gcc-11.2.1/root-5.34.38-p7sjrh5whm6m2ao3zxkaoaqomiscvw52/include/TNamed.h:26, from .sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgAttribute.h:4, from .sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgAttribute.cxx:1: /opt/rh/devtoolset-11/root/usr/include/c++/11/bits/std_abs.h:71:3: note: previous declaration 'constexpr double std::abs(double)' 71 | abs(double __x) | ^~~ In file included from .sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgBlock.h:17, from .sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgAttribute.cxx:2: .sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgMath.h:42:14: error: '_fpos_' was not declared in this scope; did you mean 'fpos_t'? 42 | #define fpos _fpos_ | ^~~~~~ In file included from /opt/software/linux-scientific7-x86_64/gcc-11.2.1/root-5.34.38-p7sjrh5whm6m2ao3zxkaoaqomiscvw52/include/Riostream.h:31, from .sl79_gcc11/include/Stiostream.h:36, from .sl79_gcc11/include/StMessMgr.h:106, from .sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgAttribute.cxx:3: /opt/rh/devtoolset-11/root/usr/include/c++/11/fstream:98:52: error: template argument 2 is invalid 98 | fpos<typename _Traits::state_type>>::value, | ^~~~~~~~~~ /opt/rh/devtoolset-11/root/usr/include/c++/11/fstream:98:66: error: '::value' has not been declared 98 | fpos<typename _Traits::state_type>>::value, | ^~~~~ cons: *** [.sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgAttribute.o] Error 1 cons: errors constructing .sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgAttribute.o ``` As suggested by @klendathu2k in star-bnl#370 (comment) we expose std::abs in StarVMC/StarAgmlLib/AgMath.h
The compiler errors are: ``` g++ -fPIC -pipe -Wall -Woverloaded-virtual -std=c++0x -Wno-long-long -pthread -Wno-deprecated-declarations -m64 -O2 -g -falign-loops -falign-jumps -falign-functions -Dsl79_gcc11 -D__ROOT__ -DNEW_DAQ_READER -I. -IStRoot -I.sl79_gcc11/include -I/opt/software/linux-scientific7-x86_64/gcc-11.2.1/root-5.34.38-p7sjrh5whm6m2ao3zxkaoaqomiscvw52/include -c .sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgAttribute.cxx -o .sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgAttribute.o In file included from .sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgBlock.h:17, from .sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgAttribute.cxx:2: .sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgMath.h:57:28: error: 'double abs(double)' conflicts with a previous declaration 57 | inline double abs (double x) {return fabs(x);} | ^ In file included from /opt/rh/devtoolset-11/root/usr/include/c++/11/cstdlib:77, from /opt/rh/devtoolset-11/root/usr/include/c++/11/ext/string_conversions.h:41, from /opt/rh/devtoolset-11/root/usr/include/c++/11/bits/basic_string.h:6607, from /opt/rh/devtoolset-11/root/usr/include/c++/11/string:55, from /opt/software/linux-scientific7-x86_64/gcc-11.2.1/root-5.34.38-p7sjrh5whm6m2ao3zxkaoaqomiscvw52/include/TSchemaHelper.h:25, from /opt/software/linux-scientific7-x86_64/gcc-11.2.1/root-5.34.38-p7sjrh5whm6m2ao3zxkaoaqomiscvw52/include/TGenericClassInfo.h:17, from /opt/software/linux-scientific7-x86_64/gcc-11.2.1/root-5.34.38-p7sjrh5whm6m2ao3zxkaoaqomiscvw52/include/Rtypes.h:270, from /opt/software/linux-scientific7-x86_64/gcc-11.2.1/root-5.34.38-p7sjrh5whm6m2ao3zxkaoaqomiscvw52/include/TObject.h:31, from /opt/software/linux-scientific7-x86_64/gcc-11.2.1/root-5.34.38-p7sjrh5whm6m2ao3zxkaoaqomiscvw52/include/TNamed.h:26, from .sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgAttribute.h:4, from .sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgAttribute.cxx:1: /opt/rh/devtoolset-11/root/usr/include/c++/11/bits/std_abs.h:71:3: note: previous declaration 'constexpr double std::abs(double)' 71 | abs(double __x) | ^~~ In file included from .sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgBlock.h:17, from .sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgAttribute.cxx:2: .sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgMath.h:42:14: error: '_fpos_' was not declared in this scope; did you mean 'fpos_t'? 42 | #define fpos _fpos_ | ^~~~~~ In file included from /opt/software/linux-scientific7-x86_64/gcc-11.2.1/root-5.34.38-p7sjrh5whm6m2ao3zxkaoaqomiscvw52/include/Riostream.h:31, from .sl79_gcc11/include/Stiostream.h:36, from .sl79_gcc11/include/StMessMgr.h:106, from .sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgAttribute.cxx:3: /opt/rh/devtoolset-11/root/usr/include/c++/11/fstream:98:52: error: template argument 2 is invalid 98 | fpos<typename _Traits::state_type>>::value, | ^~~~~~~~~~ /opt/rh/devtoolset-11/root/usr/include/c++/11/fstream:98:66: error: '::value' has not been declared 98 | fpos<typename _Traits::state_type>>::value, | ^~~~~ cons: *** [.sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgAttribute.o] Error 1 cons: errors constructing .sl79_gcc11/OBJ/StarVMC/StarAgmlLib/AgAttribute.o ``` As suggested by @klendathu2k in #370 (comment) we expose std::abs in StarVMC/StarAgmlLib/AgMath.h
cernlib was not built with GCC 11 due to wrong compiler version in the path. This is fixed in v0.1.7 of star-spack
-fno-common is the default starting gcc 10 From https://gcc.gnu.org/onlinedocs/gcc/Code-Gen-Options.html > The -fcommon places uninitialized global variables in a common block. > This allows the linker to resolve all tentative definitions of the same > variable in different compilation units to the same object, or to > a non-tentative definition. This behavior is inconsistent with C++, and > on many targets implies a speed and code size penalty on global variable > references. It is mainly useful to enable legacy code to link without > errors. Many thanks to @veprbl for the helpful suggestion: star-bnl#366 (comment)
Inspired by the following error reported while running bfc.C: ``` ... QA :INFO - Library libStEEmcUtil [ EEmcUtil] (/star-sw/.sl79_gcc11/LIB/libStEEmcUtil.so) is loaded QA :INFO - Library libStPmdUtil [ PmdUtil] (/star-sw/.sl79_gcc11/LIB/libStPmdUtil.so) is loaded dlopen error: /star-sw/.sl79_gcc11/LIB/libStEventUtilities.so: undefined symbol: gStTpcDb Load Error: Failed to load Dynamic link library /star-sw/.sl79_gcc11/LIB/libStEventUtilities.so BFC:FATAL - problem with loading of libStEventUtilities BFC:FATAL - Stu is switched off !!!! root4star: .sl79_gcc11/OBJ/StRoot/StBFChain/StBFChain.cxx:177: virtual Int_t StBFChain::Load(): Assertion `libraryload!=kStErr' failed. Error: Process completed with exit code 1. ``` Checked with `nm` utility and confirmed that gcc 11 unlike gcc 4.8.5 indeed inserts gStTpcDb into libStEventUtilities.so as undefined symbol. We also fix other macros by partially reverting changes made in 79e3801 Also, see dede9fe and ee960af
Thanks!
The context is that AgML produces both C++ and FORtran sources from the
xml inputs.
The token "abs" is a FORtran keyword, and a C/C++ function. So both the
FORtran
and C++ codes need to compile.
In principle I could have added these definitions in the C++ geometry
implementation files... the AbcdGeo.cxx files produced by the agml
parser.
I decided to collect these "hacks" inside of a single header instead, so
that
they wouldn't be forgotten. AgMath should not appear in any codes
outside of
the geometry area.
Cheers,
Jason
…On 2022-06-24 16:19, Dmitri Smirnov wrote:
@plexoos commented on this pull request.
-------------------------
In StarVMC/StarAgmlLib/AgMath.h [1]:
> @@ -54,7 +53,6 @@ Int_t nint(Float_t x);
// Fortran abs is fabs
//
//#define abs(x) TMath::Abs(x)
-inline double abs (double x) {return fabs(x);}
In general, I would oppose such change in a header file... I don't
understand why can't you call std::abs directly where it is supposed
to be called. Perhaps I am missing the real context.. but here you go
4eeac85 [2]
--
Reply to this email directly, view it on GitHub [1], or unsubscribe
[3].
You are receiving this because your review was requested.Message ID:
***@***.***>
Links:
------
[1] #370 (comment)
[2]
4eeac85
[3]
https://github.com/notifications/unsubscribe-auth/ANL4LVHEDLNEJ5YL2WUHE43VQYJ4XANCNFSM5ZVAJG7A
|
This PR branch is based on and includes changes from currently unmerged #366