CMake Updates, master branch (2019.01.25.)

This is a follow-up from !20525 (merged) and !20604 (merged).

While making sure that I would be able to compile packages that use tdaq-common on top of today's nightly, I found that I still forgot about updating a few things. Simply doing git grep "_ROOT}", I now tried to update all occurrences of "old style" variable usage to the new naming scheme.

On the VP1 packages I decided to do a bit more, and clean up the packages that used QT5_ROOT (for no good reason even in the past). @rbianchi, @emoyse, you guys should definitely have a look. The updates at least didn't break the build of these packages for me...

While doing this, I also found some additional issues in AthDataQuality, which I tried to now fix.

This merge request affects 13 packages:

• External/Herwigpp
• Generators/Epos_i
• Generators/QGSJet_i
• Projects/AthDataQuality
• Projects/AthSimulation
• Projects/Athena
• graphics/VP1/VP1AlgsBatch
• graphics/VP1/VP1AlgsEventProd
• graphics/VP1/VP1Plugins/VP1AODPlugin
• graphics/VP1/VP1Plugins/VP1GeometryPlugin
• graphics/VP1/VP1Systems/VP1AODSystems
• graphics/VP1/VP1Utils
• graphics/VP1/VP1UtilsBase

Adding @jchapman ,@vpascuzz as watchers

added Build EventDisplay Externals Generators JetEtmiss master review-pending-level-1 labels

Thanks, @akraszna for the cleanup in VP1 packages CMake setup! Changes look good. I will test them more thoroughly, later. Best, -- Ric.

CI Result SUCCESS

	Athena	AthSimulation
externals
cmake
make
required tests
optional tests

Full details available at NICOS MR-20632-2019-01-25-15-35
Athena: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
CI Jenkins server is switched to https://atlas-sit-ci.cern.ch. It is accessible world-wide (behind CERN SSO). In old links to Jenkins server aibuild080.cern.ch:8080 should be replaced with atlas-sit-ci.cern.ch For experts only: Jenkins output [CI-MERGE-REQUEST 32760]

I'm not sure, if MR shifter could approve this, as mentioned in description, expert should have a look, please @emoyse , could you approve these changes ?

Thanks, Pavol [as L2 MR shifter]

added review-pending-expert label and removed review-pending-level-1 label

Since Riccardo didn't seem to mind the VP1 package configuration updates, I guess at this point the question is if @tsulaia and @ssnyder trust me with these updates or not.

I'll just wait for one of them to merge this at one point...

Hi @akraszna, sorry, I forgot to answer here: yes, I'm fine with the changes. Actually, thanks for the cleanup!

Only one minor question, mainly out of curiosity: I noticed you removed the occurences of ${pkgName}, which I had used to set the name of the package only once at the beginning of the file. I thought that could help to reduce typos and for reuse of CMake code. Is that considered poor practice with CMake?

Thanks, -- Ric.

@akraszna I always trust your updates

Only one minor question, mainly out of curiosity: I noticed you removed the occurences of ${pkgName}, which I had used to set the name of the package only once at the beginning of the file. I thought that could help to reduce typos and for reuse of CMake code. Is that considered poor practice with CMake?

First off, using atlas_get_package_name was really unnecessarry. You’ve set the name of the package just a few lines before that call.

And yeah, I didn’t like this setup too much. Unlike with cmt, we are not bound by naming conventions nearly as much with cmake. So I prefer spelling out what library names we use. Especially becase in many cases the library names are actually different than the name of the package.

Vakho, I prefer not merging stuff into master myself. So I’ll wait for somebody else with the merge...

Hi Attila, according to our OTP page nobody is taking master RC shifts this week. OTOH, I see that Frank merged a bunch of MRs earlier today, so I don't really want to interfere. It is already late for r28, so I'd rather wait until tomorrow morning CA time. Then if I find that it has not yet been merged, I'll do that myself.

I see @fwinkl listed also for today.

@akraszna should this be merged?

I think so, yes.

Will mark as approved then and also add RC Attention Required.

added RC Attention Required review-approved labels and removed review-pending-expert label

merged

mentioned in commit f8d039fd

First off, using atlas_get_package_name was really unnecessarry. You’ve set the name of the package just a few lines before that call.

And yeah, I didn’t like this setup too much. Unlike with cmt, we are not bound by naming conventions nearly as much with cmake. So I prefer spelling out what library names we use. Especially becase in many cases the library names are actually different than the name of the package.

Thanks for the detailed explanations, @akraszna !

added sweep:ignore label