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

modernize MuonAnalysis/MomentumScaleCalibration #36485

Merged

Conversation

mmusich
Copy link
Contributor

@mmusich mmusich commented Dec 14, 2021

PR description:

Part of the migration in #31061 and #36404
Went systematically through all of the CMSDEPRECATED_X warnings in the MuonAnalysis/MomentumScaleCalibration subsystem from CMSSW_12_3_CMSDEPRECATED_X_2021-12-10-2300 and removed deprecated API calls.
Bonus:

  • merged header files in plugins implementations

PR validation:

cmssw compiles

if this PR is a backport please specify the original PR and why you need to backport that PR:

N/A

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36485/27359

  • This PR adds an extra 36KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mmusich (Marco Musich) for master.

It involves the following packages:

  • MuonAnalysis/MomentumScaleCalibration (analysis)

@cmsbuild, @santocch can you please review it and eventually sign? Thanks.
@rchatter, @bellan, @argiro, @thomreis, @simonepigazzini this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36485/27360

  • This PR adds an extra 36KB to repository

@cmsbuild
Copy link
Contributor

Pull request #36485 was updated. @cmsbuild, @santocch can you please check and sign again.

@mmusich
Copy link
Contributor Author

mmusich commented Dec 14, 2021

please test

@cmsbuild
Copy link
Contributor

-1

Failed Tests: Build HeaderConsistency ClangBuild
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9561e5/21253/summary.html
COMMIT: d9ac203
CMSSW: CMSSW_12_3_X_2021-12-13-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36485/21253/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9561e5/21253/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9561e5/21253/git-merge-result

Build

I found compilation error when building:

/cvmfs/cms-ib.cern.ch/nweek-02711/slc7_amd64_gcc900/external/sigcpp/2.6.2-cms/include/sigc++-2.0/sigc++/functors/slot.h:1655:7:   required from 'class sigc::slot'
/cvmfs/cms-ib.cern.ch/nweek-02711/slc7_amd64_gcc900/external/sigcpp/2.6.2-cms/include/sigc++-2.0/sigc++/signal.h:658:41:   required from 'struct sigc::internal::signal_emit0'
/cvmfs/cms-ib.cern.ch/nweek-02711/slc7_amd64_gcc900/external/sigcpp/2.6.2-cms/include/sigc++-2.0/sigc++/signal.h:2635:54:   required from 'class sigc::signal0'
/cvmfs/cms-ib.cern.ch/nweek-02711/slc7_amd64_gcc900/external/sigcpp/2.6.2-cms/include/sigc++-2.0/sigc++/signal.h:3839:7:   required from 'class sigc::signal'
/data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_3_X_2021-12-13-1100/src/Fireworks/Core/interface/FWViewContext.h:42:52:   required from here
/cvmfs/cms-ib.cern.ch/nweek-02711/slc7_amd64_gcc900/external/sigcpp/2.6.2-cms/include/sigc++-2.0/sigc++/functors/slot.h:431:22: error: function returning a function
  431 |   typedef T_return (*call_type)(rep_type*);
      |                      ^~~~~~~~~
/cvmfs/cms-ib.cern.ch/nweek-02711/slc7_amd64_gcc900/external/sigcpp/2.6.2-cms/include/sigc++-2.0/sigc++/functors/slot.h:437:19: error: function returning a function
  437 |   inline T_return operator()() const
      |                   ^~~~~~~~


Clang Build

I found compilation error while trying to compile with clang. Command used:

USER_CUDA_FLAGS='--expt-relaxed-constexpr' USER_CXXFLAGS='-Wno-register -fsyntax-only' scram build -k -j 32 COMPILER='llvm compile'

>> Entering Package PhysicsTools/PythonAnalysis
>> Entering Package RecoEgamma/EgammaTools
>> Entering Package RecoVertex/BeamSpotProducer
>> Entering Package SimTransport/PPSProtonTransport
>> Compile sequence completed for CMSSW CMSSW_12_3_X_2021-12-13-1100
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 1
+ eval scram build outputlog '&&' '(python3' /data/cmsbld/jenkins/workspace/ib-run-pr-tests/cms-bot/buildLogAnalyzer.py --ignoreWarning=Wdeprecated-declarations --logDir /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_3_X_2021-12-13-1100/tmp/slc7_amd64_gcc900/cache/log/src '||' 'true)'
++ scram build outputlog
>> Entering Package Alignment/OfflineValidation
>> Compiling  /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_12_3_X_2021-12-13-1100/src/Alignment/OfflineValidation/bin/DMRtrends.cc
Entering library rule at src/Alignment/OfflineValidation/plugins


@mmusich
Copy link
Contributor Author

mmusich commented Dec 14, 2021

@cms-sw/orp-l2 is the release broken?
I don't see how my changes can cause the compilation errors at #36485 (comment)
Also I see the same at: #36484

@mmusich mmusich closed this Dec 14, 2021
@mmusich mmusich reopened this Dec 14, 2021
@perrotta
Copy link
Contributor

@mmusich #36343 and cms-sw/cmsdist#7441 may have broken it: investigating...

@qliphy
Copy link
Contributor

qliphy commented Dec 14, 2021

@cms-sw/orp-l2 is the release broken? I don't see how my changes can cause the compilation errors at #36485 (comment) Also I see the same at: #36484

As shown above, #36343 was also included on top of IB + this PR after doing git cms-merge-topic ,
and we need also cms-sw/cmsdist#7441 which has been included in next IB: CMSSW_12_3_X_2021-12-13-2300

@mmusich
Copy link
Contributor Author

mmusich commented Dec 14, 2021

@qliphy @perrotta thanks!
So it's just a matter of waiting for the next IB to appear?

@qliphy
Copy link
Contributor

qliphy commented Dec 14, 2021

@qliphy @perrotta thanks! So it's just a matter of waiting for the next IB to appear?

Yes, I think so. We need to wait for next IB with cms-sw/cmsdist#7441

@perrotta
Copy link
Contributor

It seems to be there now: https://cmssdt.cern.ch/SDT/html/cmssdt-ib/#/ib/CMSSW_12_3_X (I don't know if it was already there when you tested first 2 hours ago)
To stay on the safe side, we can even test with the missing piece...

@perrotta
Copy link
Contributor

please test with cms-sw/cmsdist#7441

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9561e5/21258/summary.html
COMMIT: d9ac203
CMSSW: CMSSW_12_3_X_2021-12-13-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36485/21258/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 42
  • DQMHistoTests: Total histograms compared: 3250704
  • DQMHistoTests: Total failures: 5
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3250676
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.004 KiB( 41 files compared)
  • DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
  • Checked 177 log files, 37 edm output root files, 42 DQM output files
  • TriggerResults: no differences found

@qliphy
Copy link
Contributor

qliphy commented Dec 16, 2021

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9561e5/21301/summary.html
COMMIT: d9ac203
CMSSW: CMSSW_12_3_X_2021-12-15-2300/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/36485/21301/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 42
  • DQMHistoTests: Total histograms compared: 3250719
  • DQMHistoTests: Total failures: 11
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3250685
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.004 KiB( 41 files compared)
  • DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
  • Checked 177 log files, 37 edm output root files, 42 DQM output files
  • TriggerResults: no differences found

@qliphy
Copy link
Contributor

qliphy commented Dec 19, 2021

merge

@cmsbuild cmsbuild merged commit cc11e36 into cms-sw:master Dec 19, 2021
@mmusich mmusich deleted the modernizeMuonAnalysisMomentumScaleCalibration branch December 19, 2021 09:09
@santocch
Copy link

+1

@cmsbuild
Copy link
Contributor

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 be automatically merged.

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

5 participants