-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Use named constants for uG(M)T firmware versions etc. #40841
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40841/34297
|
A new Pull Request was created by @dinyar (Dinyar Rabady) for master. It involves the following packages:
@epalencia, @cmsbuild, @cecilecaillol, @aloeliger can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
static constexpr int ugmtFedId_ = 1402; | ||
static constexpr int ugtFedId_ = 1404; | ||
static constexpr int ugmtFwVersionUntil2016_ = 0x4010000; | ||
static constexpr int ugtFwVersionUntil2016_ = 0x10A6; | ||
static constexpr int ugmtFwVersionUntil2017_ = 0x6000000; | ||
static constexpr int ugtFwVersionUntil2017_ = 0x1120; | ||
static constexpr int ugmtFwVersionRun3Start_ = 0x6000001; | ||
static constexpr int ugtFwVersionUntilRun3Start_ = 0x1130; | ||
static constexpr int ugmtFwVersionFirstWithShowers_ = 0x7000000; | ||
static constexpr int ugtFwVersionFirstWithShowers_ = 0x113B; |
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.
Perhaps someone can correct me on this, but I think it is preferred that if you are adding class constants, that they have prefix k
in front of them?
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.
Hi Andrew,
Ah, didn't know that but I'm happy to change it. Should I then do so for the other constants in that file too, I suppose? Also, is it fine to only do it in this PR, but not for the backport?
Cheers,
Dinyar
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.
@dinyar It should be fine to keep old constants how they are for now, but I believe that any newly added constants should have prefix k
according to http://cms-sw.github.io/cms_coding_rules.html
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.
@dinyar And if possible I think it would be good to have the change (same commit if it all possible) in both this and the backport.
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.
Hi @aloeliger,
Ok, done now, let me know if it's ok. I've created a separate commit for the change, but if it also goes in the backport I'd be tempted to squash it (and do the same for the backport). Is that fine?
-d
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.
@dinyar I would think it's fine. The alternative is you could just git cherry-pick
the same commit to the backport branch of your repository.
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.
Ok, done.
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d6be00/30792/summary.html 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: Comparison SummarySummary:
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40841/34306
|
Pull request #40841 was updated. @epalencia, @cmsbuild, @cecilecaillol, @aloeliger can you please check and sign again. |
Please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d6be00/30817/summary.html Comparison SummarySummary:
|
Also fix overlooked incorrect uGT version from previous commit.
7a51b51
to
dd8a1f3
Compare
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40841/34315
|
Pull request #40841 was updated. @epalencia, @cmsbuild, @cecilecaillol, @aloeliger can you please check and sign again. |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d6be00/30826/summary.html Comparison SummaryThere are some workflows for which there are errors in the baseline: Summary:
|
+l1 |
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. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
Use named constants for the uG(M)T firmware versions and FED IDs used by the (un)packers. Also fixed an oversight spotted by @elfontan where I had forgotten to correct the uGT firmware version in one spot.