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

Fix consumes #1231

Merged
merged 45 commits into from Nov 20, 2013
Merged

Fix consumes #1231

merged 45 commits into from Nov 20, 2013

Conversation

rovere
Copy link
Contributor

@rovere rovere commented Oct 30, 2013

First bunch of getByToken changes in DQM packages.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @rovere (Marco Rovere) for CMSSW_7_0_X.

Fix consumes

It involves the following packages:

DQM/BeamMonitor
DQM/Physics
DQM/DTMonitorModule

@smuzaffar, @nclopezo, @danduggan, @rovere, @deguio, @eliasron can you please review it and eventually sign? Thanks.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
@ktf you are the release manager for this.

@nclopezo
Copy link
Contributor

-1
When I ran the usual tests I found the following errors in the RelVals in the following workflows:

4.22 step2
1001.0 step2
1000.0 step2

----- Begin Fatal Exception 30-Oct-2013 17:18:18 CET-----------------------
An exception of category 'Configuration' occurred while
   [0] Constructing the EventProcessor
   [1] Constructing module: class=DTLocalTriggerSynchTask label='dtTriggerSynchMonitor'
Exception Message:
MissingParameter: Parameter 'DCCInputTag' not found.
----- End Fatal Exception -------------------------------------------------

you can see the results of the tests here:

https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/1090/summary.html

@cmsbuild
Copy link
Contributor

Pull request #1231 was updated. @smuzaffar, @thspeer, @demattia, @danduggan, @rovere, @nclopezo, @rcastello, @deguio, @slava77, @eliasron can you please check and sign again.

@cmsbuild
Copy link
Contributor

@ktf
Copy link
Contributor

ktf commented Oct 31, 2013

Marco your stuff does not merge anymore. Can you rebase it, please?

@rovere
Copy link
Contributor Author

rovere commented Oct 31, 2013

oh my god, back to CVS conflicts....

I'll rebase it.

M.

@ktf
Copy link
Contributor

ktf commented Oct 31, 2013

git rebase -i is your friend.

@rovere
Copy link
Contributor Author

rovere commented Oct 31, 2013

git rebase CMSSW_7_0_X is even easier. the problem was related to a new plugin that went in into DQM/Physics.

I'll test-again the pull and then I'll push my changes to my local repo, which will eventually be caught automatically here.

Ciao,
Marco.

@cmsbuild
Copy link
Contributor

Pull request #1231 was updated. @smuzaffar, @thspeer, @demattia, @danduggan, @rovere, @nclopezo, @rcastello, @deguio, @slava77, @eliasron can you please check and sign again.

@cmsbuild
Copy link
Contributor

@thspeer
Copy link
Contributor

thspeer commented Nov 1, 2013

Working @thspeer

@@ -148,3 +148,8 @@ class BSFitter {

#endif


/* Local Variables: */
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the meaning of these lines? I see that in nearly every file here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's simply custom-emacs settings. At some point I got bored of seeing ugly{,-formatted} code....

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not put them into your emacs config file ? We can't pollute every CMSSW source file with our own emacs settings !

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the ugly formatted code?

I've been in the emacs land since at least Y2K, never needed to add any junk to the source code to get something better out of emacs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'Cause I edit CMSSW in 3 different envs and I do not have all of my .emacs in sync.

Anyway, I have no intention to start a religious war about editing/formatting and similar things, since the code is there to demonstrate we miserably failed on this.

Feel free to take them out and resubmit a pull request, if you deem so: for sure I do not want to discuss stylistic emacs-directives in comments.

By the way: getting rid of useless spaces at the end of a line is, IMHO, a good practice in general that I hoped would have been taken by everybody.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Guys, are we serious or what?
If you do not like the comments, git rid of them but please stop discussing about them.
We can invest better our working hours.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ciao Marco,

Cleaning up of empty spaces and good indentation is really good.

Comments which apply to the code are also very good.

Text which points to nothing and has to be dragged around is bad.
What are these settings in emacs that you really need to have these
comments?
Could you please send a link or smth.

What if some other editors (vi, nedit, pico etc) need something like
that as well,
would we then go cycling over different preferences?

Please remove them.

Cheers

    --slava

On 11/1/13, 2:24 PM, Marco Rovere wrote:

In RecoVertex/BeamSpotProducer/interface/BSFitter.h:

@@ -148,3 +148,8 @@ class BSFitter {

#endif

+/* Local Variables: */

Guys, are we serious or what?
If you do not like the comments, git rid of them but please stop
discussing about them.
We can invest better our working ours.


Reply to this email directly or view it on GitHub
https://github.com/cms-sw/cmssw/pull/1231/files#r7368821.


Vyacheslav (Slava) Krutelyov
TAMU: Physics Dept Texas A&M MS4242, College Station, TX 77843-4242
CERN: 42-R-027
AIM/Skype: siava16 googleTalk: slava77@gmail.com
(630) 291-5128 Cell (US) +41 76 275 7116 Cell (CERN)


Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dear Slava, all
unless you start an effort on removing similar customisations across full CMSSW, I'll not remove them.
By the way, we have already something similar for emacs, and a couple for both vim and vi.

If you don't like my pull, give a -1.

M.

Copy link
Contributor

Choose a reason for hiding this comment

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

On 11/1/13, 3:02 PM, Marco Rovere wrote:

In RecoVertex/BeamSpotProducer/interface/BSFitter.h:

@@ -148,3 +148,8 @@ class BSFitter {

#endif

+/* Local Variables: */

Dear Slava, all
unless you start an effort on removing similar customisations across
full CMSSW, I'll not remove them.
By the way, we have already something similar for emacs, and a couple
for both vim and vi.

If you don't like my pull, give a -1.

Dear Marco,

I'm just trying to understand the need or use cases for these.
I tried to check in lxr
https://cmssdt.cern.ch/SDT/lxr/search?v=CMSSW_7_0_0_pre6;string=show-trailing
and I don't see anything like the one from you in the release.
Which other customizations for editors did you have in mind.
Please clarify.

Thank you.

Cheers

    --slava

M.


Reply to this email directly or view it on GitHub
https://github.com/cms-sw/cmssw/pull/1231/files#r7369520.


Vyacheslav (Slava) Krutelyov
TAMU: Physics Dept Texas A&M MS4242, College Station, TX 77843-4242
CERN: 42-R-027
AIM/Skype: siava16 googleTalk: slava77@gmail.com
(630) 291-5128 Cell (US) +41 76 275 7116 Cell (CERN)


@deguio
Copy link
Contributor

deguio commented Nov 3, 2013

+1

@deguio
Copy link
Contributor

deguio commented Nov 8, 2013

ciao all,
any chance of having this one included in 700_pre9?
could the responsible test and sign (or reject if it is the case)?

thanks,
F.

@ktf
Copy link
Contributor

ktf commented Nov 8, 2013

It also needs rebase by now...

@thspeer
Copy link
Contributor

thspeer commented Nov 8, 2013

-1
We can not have personal formatting in every file like this

@cmsbuild
Copy link
Contributor

Pull request #1231 was updated. @thspeer, @demattia, @danduggan, @rovere, @cmsbuild, @nclopezo, @rcastello, @deguio, @slava77, @eliasron can you please check and sign again.

@rovere
Copy link
Contributor Author

rovere commented Nov 11, 2013

This is an updated version of my original pull.

@thspeer + @slava77: please take some time to review the changes I made related to the transition out of TFitterMinuit in ROOT6. From some preliminary test I made locally, it seems that everything is fine, but I'd appreciate a second, independent check.

@ktf @deguio @danduggan

@deguio
Copy link
Contributor

deguio commented Nov 12, 2013

+1

@ktf
Copy link
Contributor

ktf commented Nov 12, 2013

How can you +1 something which does not merge? ;-) Please rebase.

@deguio
Copy link
Contributor

deguio commented Nov 12, 2013

ciao @ktf
it merges on top of CMSSW_7_0_X_2013-11-11-0200

am I doing something wrong?

@ktf
Copy link
Contributor

ktf commented Nov 12, 2013

No, but life goes on and we are on the 12… ;-)

@cmsbuild
Copy link
Contributor

Pull request #1231 was updated. @thspeer, @demattia, @danduggan, @rovere, @cmsbuild, @nclopezo, @rcastello, @deguio, @slava77, @eliasron can you please check and sign again.

@rovere
Copy link
Contributor Author

rovere commented Nov 12, 2013

Let's keep on rebasing it 'till it got approved ;)
@ktf @deguio @danduggan

@ktf
Copy link
Contributor

ktf commented Nov 12, 2013

BTW, is the java stuff in DQM/BeamMonitor really used?

@deguio
Copy link
Contributor

deguio commented Nov 12, 2013

yes, it is used for the online at the moment.

@deguio
Copy link
Contributor

deguio commented Nov 12, 2013

+1
tested on top of CMSSW_7_0_X_2013-11-12-0200 this time

@cmsbuild
Copy link
Contributor

@thspeer
Copy link
Contributor

thspeer commented Nov 13, 2013

+1
tested cf5ed49 in CMSSW_7_0_X_2013-11-13-0200-1231
No change in reco.
The changes for TFitterMinuit Migration for ROOT6 are not run in the standard workflows, and will be tested by Kevin B, but on a longer timescale. In order not to hold all the developments back, I signed it.

@deguio
Copy link
Contributor

deguio commented Nov 20, 2013

ciao @demattia @rcastello
do you mind signing this one?

thanks,
F.

@rcastello
Copy link

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next IBs unless changes or unless it breaks tests. @ktf can you please take care of it?

ktf added a commit that referenced this pull request Nov 20, 2013
Consumes migration -- Fix consumes in CMSSW
@ktf ktf merged commit 861d03d into cms-sw:CMSSW_7_0_X Nov 20, 2013
shervin86 pushed a commit to shervin86/cmssw that referenced this pull request May 11, 2015
Consumes migration -- Fix consumes in CMSSW
@rovere rovere deleted the fix-consumes branch October 3, 2023 07:40
aloeliger added a commit to aloeliger/cmssw that referenced this pull request Apr 7, 2024
…Debug

L1TrackVertexAssociationProducer bug fix to stop accessing data after move
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