-
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
Fix typo in L1T prescales files #37582
Conversation
urgent
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37582/29338
|
type bug-fix |
A new Pull Request was created by @francescobrivio for master. It involves the following packages:
@epalencia, @cmsbuild, @cecilecaillol, @rekovic can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild please test |
@cms-sw/orp-l2 given that nothing tests this in CMSSW (otherwise we would have noticed this issue) is there a point in waiting for the tests to finish? |
It seems we still need to wait for comment from https://github.com/orgs/cms-sw/teams/l1-l2 |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b53d74/23952/summary.html Comparison SummarySummary:
|
Pinging explicitly @panoskatsoulis in case the comment of Qiang was not noticed. |
I needed to overcome some authentication issues |
So, this commit included here fixes the issue that occurred yesterday. However, seems there is an other issue that was hiding behind this.
Here is the latest log from Prep |
@panoskatsoulis who exactly is supposed to look at this problem? |
@francescobrivio , all: do I understand correctly that this fix is correct and should eventually get integrated, BUT another independent bug was discovered that also needs to be fixed? |
Hi, yes this typo fix is needed in any case. (We may merge it) The above error is not L1 related,
seems that this is a transaction issue related to the
I didn't look into this further because of the following problem that came up. As Tamas says we need @ggovi for this. However for L1 there is an other issue as well that was not expected. Here is the latest log from prep (after reverting the above commits to make the transaction again working) |
Was this #36383 (comment) eventually followed up by the L1 team? |
not that I know, let me have a look. |
@perrotta I agree that this should be merged in any case and then we can move on to fixing the other issue(s). |
@panoskatsoulis @francescobrivio To give an updated summary as below. Let me know if there is anything missing:
|
+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, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
@francescobrivio we will need a backport for the to 12_3 |
Hey @panoskatsoulis, |
ah yes - I didnt notice |
PR description:
This PR fixes a typo introduced in #37453 by adding back the
.xml
string in the L1T prescale files.The typo is a blocker for the correct operations of the L1T O2Os so it should be fixed asap.
PR validation:
Code compiles.
@cms-sw/l1-l2 should comment further if other tests are needed.
Backport:
Not a backport. A backport to 12_3_X is available in #37583
FYI: @dpiparo @cms-sw/db-l2 @panoskatsoulis @elfontan @boudoul