Skip to content
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

Removed FWCore/ParameterSet dependecies from *Formats packages #33897

Merged
merged 6 commits into from Jun 3, 2021

Conversation

Dr15Jones
Copy link
Contributor

PR description:

Removed FWCore/ParameterSet dependencies from *Formats packages. As a general rule, no data products (ie. classes stored in *Formats subsystems) should have any dependency upon the framework itself, which includes how we configure the framework (i.e. the ParameterSet system).

This was prompted by the fact that the CXXMODULES IBs are failing because of ROOT's attempt to create a module from FWCore/ParameterSet. That failure caused many *Formats packages to also fail. If we remove the unnecessary dependency than we should be able to avoid having ROOT even attempt the parsing.

PR validation:

The code compiles.

This is a step towards decreasing dependencies on FWCore/ParameterSet
in Formats packages.
This allowed removal of FWCore/ParameterSet dependency from
CondFormats/CSCObjects and some tests.
This allowed those dependencies to be dropped from BuildFile.xml.
@@ -24,7 +24,7 @@ class SiStripDeDx2DBuilder : public edm::EDAnalyzer {
virtual void analyze(const edm::Event&, const edm::EventSetup&);

private:
edm::FileInPath fp_;
//edm::FileInPath fp_;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: this was left intentionally commented out as the code that uses fp_ was already commented out.

@@ -24,7 +24,7 @@ class SiStripDeDx3DBuilder : public edm::EDAnalyzer {
virtual void analyze(const edm::Event&, const edm::EventSetup&);

private:
edm::FileInPath fp_;
//edm::FileInPath fp_;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: this was left intentionally commented out as the code that uses fp_ was already commented out.

@@ -22,7 +22,7 @@ class SiStripDeDxMipBuilder : public edm::EDAnalyzer {
virtual void analyze(const edm::Event&, const edm::EventSetup&);

private:
edm::FileInPath fp_;
//edm::FileInPath fp_;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: this was left intentionally commented out as the code that uses fp_ was already commented out.

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33897/22960

ERROR: Build errors found during clang-tidy run.

Suppressed 507 warnings (506 in non-user code, 1 with check filters).
--
DataFormats/PatCandidates/src/CovarianceParameterization.cc:12:10: error: 'FWCore/Utilities/interface/FileInPath.h' file not found [clang-diagnostic-error]
#include "FWCore/Utilities/interface/FileInPath.h"
         ^
Suppressed 1134 warnings (1133 in non-user code, 1 with check filters).
--
CondFormats/SiPixelTransient/src/SiPixelTemplate2D.cc:41:10: error: 'FWCore/Utilities/interface/FileInPath.h' file not found [clang-diagnostic-error]
#include "FWCore/Utilities/interface/FileInPath.h"
         ^
Suppressed 508 warnings (507 in non-user code, 1 with check filters).
--
CondFormats/PPSObjects/src/PPSDirectSimulationData.cc:8:10: error: 'FWCore/Utilities/interface/FileInPath.h' file not found [clang-diagnostic-error]
#include "FWCore/Utilities/interface/FileInPath.h"
         ^
Suppressed 800 warnings (799 in non-user code, 1 with check filters).
--
CondFormats/SiPixelTransient/src/SiPixelTemplate.cc:107:10: error: 'FWCore/Utilities/interface/FileInPath.h' file not found [clang-diagnostic-error]
#include "FWCore/Utilities/interface/FileInPath.h"
         ^
Suppressed 507 warnings (506 in non-user code, 1 with check filters).
--
gmake: *** [config/SCRAM/GMake/Makefile.coderules:128: code-checks] Error 2
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 2

@Dr15Jones
Copy link
Contributor Author

code-checks

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33897/22965

  • This PR adds an extra 96KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Dr15Jones (Chris Jones) for master.

It involves the following packages:

CondFormats/CSCObjects
CondFormats/JetMETObjects
CondFormats/L1TObjects
CondFormats/PPSObjects
CondFormats/PhysicsToolsObjects
CondFormats/SiPixelObjects
CondFormats/SiPixelTransient
DataFormats/PatCandidates
EventFilter/CSCRawToDigi

@perrotta, @malbouis, @yuanchao, @jpata, @tlampen, @cmsbuild, @rekovic, @slava77, @ggovi, @pohsun, @cecilecaillol, @francescobrivio can you please review it and eventually sign? Thanks.
@rappoccio, @gouskos, @hatakeyamak, @Martin-Grunewald, @OzAmram, @seemasharmafnal, @mmarionncern, @ahinzmann, @jdolen, @dildick, @barvic, @rovere, @VinInn, @jdamgov, @ferencek, @nhanvtran, @gkasieczka, @tocheng, @schoef, @mmusich, @ptcox, @clelange, @jan-kaspar, @dkotlins, @cbernet, @gpetruc, @mariadalfonso, @tvami this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@cecilecaillol
Copy link
Contributor

+l1

@cmsbuild
Copy link
Contributor

Pull request #33897 was updated. @perrotta, @malbouis, @yuanchao, @jpata, @tlampen, @cmsbuild, @rekovic, @slava77, @ggovi, @pohsun, @cecilecaillol, @francescobrivio can you please check and sign again.

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9c2aa7/15479/summary.html
COMMIT: 0c333fc
CMSSW: CMSSW_12_0_X_2021-05-31-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33897/15479/install.sh to create a dev area with all the needed externals and cmssw changes.

CMS Clang-Tidy warnings: There are Clang-Tidy warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9c2aa7/15479/llvm-analysis/cmsclangtidy.txt for details.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2650486
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2650463
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
  • Checked 155 log files, 37 edm output root files, 37 DQM output files
  • TriggerResults: no differences found

@ggovi
Copy link
Contributor

ggovi commented May 31, 2021

+1

@cecilecaillol
Copy link
Contributor

+l1

@slava77
Copy link
Contributor

slava77 commented Jun 2, 2021

+reconstruction

for #33897 0c333fc

  • changes in reco are technical
  • there are no differences in comparisons

@yuanchao
Copy link
Contributor

yuanchao commented Jun 2, 2021

+1

  • removed FWCore/ParameterSet dependencies from *Formats packages
  • matrix tests and unit tests passed

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 2, 2021

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@qliphy
Copy link
Contributor

qliphy commented Jun 3, 2021

+1

@cmsbuild cmsbuild merged commit 1ae7916 into cms-sw:master Jun 3, 2021
@Dr15Jones Dr15Jones deleted the fixParameterSetDepsInFormats branch June 3, 2021 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants