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

[VISUALIZATION] Apply code-checks/format with misc-definitions-in-headers #34054

Merged
merged 2 commits into from Jun 10, 2021
Merged

[VISUALIZATION] Apply code-checks/format with misc-definitions-in-headers #34054

merged 2 commits into from Jun 10, 2021

Conversation

smuzaffar
Copy link
Contributor

code-checks/format applied on full cmssw

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34054/23188

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 8, 2021

A new Pull Request was created by @smuzaffar (Malik Shahzad Muzaffar) for master.

It involves the following packages:

Fireworks/Core

@makortel, @alja, @cmsbuild, @Dr15Jones can you please review it and eventually sign? Thanks.
@alja 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

@alja
Copy link
Contributor

alja commented Jun 8, 2021

@smuzaffar Thank You!
+1

"r"
"s")(kExpertCommandOpt,
"Enable PF user plugins.")(kHelpCommandOpt,
"Display help message");
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say -1 though...

Copy link
Contributor Author

@smuzaffar smuzaffar Jun 9, 2021

Choose a reason for hiding this comment

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

agree, we should add

// clang-format off

// clang-format on

around this section

@alja
Copy link
Contributor

alja commented Jun 8, 2021

@makortel
@smuzaffar
Is there a way to tell formatter to leave out some code?

@makortel
Copy link
Contributor

makortel commented Jun 8, 2021

There is, but we have the same issue on many exetutables in FWCore, so let's figure out a common solution first.

@smuzaffar
Copy link
Contributor Author

@makortel , I think there is no automatic solution. We do want clang-format to format the code and wrap the lines to 120 chars. I would suggest that so cases like this and few in Core we just wrap the section with

// clang-format off

// clang-format on

@makortel
Copy link
Contributor

makortel commented Jun 9, 2021

@makortel , I think there is no automatic solution. We do want clang-format to format the code and wrap the lines to 120 chars. I would suggest that so cases like this and few in Core we just wrap the section with

// clang-format off

// clang-format on

Ok (pretty much what I was afraid of). How do we proceed in practice? Should we format these sections manually now (that they're not that awful yet) and repeat the automatic formatting, or merge these PRs and format manually afterwards?

@smuzaffar
Copy link
Contributor Author

@makortel , yes I can fix this PR and one for core ( #34036 ) and revert the formatting changes ( where it is really awful) and add // clang-format off/on around those sections.

@makortel
Copy link
Contributor

makortel commented Jun 9, 2021

Thanks @smuzaffar!

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 9, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34054/23210

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 9, 2021

Pull request #34054 was updated. @makortel, @alja, @cmsbuild, @Dr15Jones can you please check and sign again.

@smuzaffar
Copy link
Contributor Author

please test

@alja
Copy link
Contributor

alja commented Jun 9, 2021

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 9, 2021

This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. 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)

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 9, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-899b35/15822/summary.html
COMMIT: 2d9f96c
CMSSW: CMSSW_12_0_X_2021-06-08-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/34054/15822/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: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 38
  • DQMHistoTests: Total histograms compared: 2862520
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2862497
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 37 files compared)
  • Checked 160 log files, 37 edm output root files, 38 DQM output files
  • TriggerResults: no differences found

@qliphy
Copy link
Contributor

qliphy commented Jun 10, 2021

+1

@cmsbuild cmsbuild merged commit e9a85b5 into cms-sw:master Jun 10, 2021
@smuzaffar smuzaffar deleted the 12_0-code-checks-VISUALIZATION branch June 15, 2021 07:09
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