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

fillDescriptions migration and parameter cleanup in DAQ modules (80X) #12148

Merged
merged 5 commits into from Nov 5, 2015

Conversation

smorovic
Copy link
Contributor

Changes:

  • Migrating production DAQ modules EvFDaqDirector, EvFOutputModule, FedRawDataInputSource and FastMonitoringService to using fillDescriptions. Removing cfi python files for those cases.
  • Removing obsolete parameters from those modules
  • Allow FastMonitoringService to find definition file in both CMSSW_BASE and CMSSW_RELEASE_BASE (those differ when running from project area).

Port of #12146 (75X) and #12147 (76X)

*ShmStreamConsumer as legacy name remains defined in ShmStreamConsumer_cfi.py
…LEASE_BASE and local path). Parameters for this definition are no longer exported.

*file prefixes for fast and slow monitoring do not need to be configurable
@slava77
Copy link
Contributor

slava77 commented Oct 28, 2015

@cmsbuild please test

@cmsbuild cmsbuild added this to the Next CMSSW_8_0_X milestone Oct 28, 2015
@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @smorovic (Srecko Morovic) for CMSSW_8_0_X.

fillDescriptions migration and parameter cleanup in DAQ modules (80X)

It involves the following packages:

EventFilter/Utilities

@mommsen, @cvuosalo, @cmsbuild, @emeschi, @slava77 can you please review it and eventually sign? Thanks.
@Martin-Grunewald this is something you requested to watch as well.
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.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@cmsbuild
Copy link
Contributor

The jenkins tests job failed, please try again.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/9270/console

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

@cvuosalo
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/9399/console

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 3, 2015

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 3, 2015

@smorovic
Copy link
Contributor Author

smorovic commented Nov 3, 2015

I applied updates based on some of the changes. Here is a summary of those that still remain:

Dereference of null pointer in ExceptionGenerator.cc
* module intentionally dereferences null pointer (twice) to crash as part of the test. we could probably fool the checker here by "calculating" 0.

non-const global static variables in crc32c.cc:
• these are all initialized with pthread_once (thread safe)

non-const (global) static variable in json_value.cpp
• json library intializes some references during initialization of static global data, before main(). As far as I understand, this is safe.
• more modern version of the library doesn't seem to do this: https://github.com/open-source-parsers/jsoncpp/blob/master/src/lib_json/json_value.cpp
so eventually we could migrate to it.

"inherits from edm::EDProducer,edm::EDFilter,edm::EDAnalyzer, or edm::OutputModule"
• some modules are not yet ported to the new interfaces (not used in production, but for testing or debugging raw data). They could probably easily be converted to "one" interfaces, but I didn't do it at this time.

non-const static variable in EventFilter/Utilities/plugins/RawEventFileWriterForBU.cc
• this module used only for testing purpose. static member pointer provides member data access to the custom signal handler

const_cast used on a pointer to a data class EventFilter /Utilities/src /json_value.cpp
• same variable is used to store const and non-const variant (a boolean flag set differently for two cases). Non-trivial to fix in the json library
- to just silence the test, probably it can be achieved with putting char * and const char* in union and use either one where appropriate....

Also I modifies this one, but static checker still complains:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12148/9436/llvm-analysis/report-5a0da4.html#EndPath

@mommsen
Copy link
Contributor

mommsen commented Nov 4, 2015

+1

@cvuosalo
Copy link
Contributor

cvuosalo commented Nov 4, 2015

@smorovic: It looks like you may have missed a simple const problem in FastMonitoring.cc, line 116:

legendaVector.append(Json::Value(*((std::string *)(encPath_[0].decode(i)))));

Won't const std:string * fix this one?

https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-12148/9436/llvm-analysis/report-2ae9d4.html#EndPath

@smorovic
Copy link
Contributor Author

smorovic commented Nov 4, 2015

@cvuosalo,
that one was changed in other PR (using C++ style casts).
Here is the change:
https://github.com/cms-sw/cmssw/pull/12245/files#diff-6868dae040be2082e8a0d79168e37c1bR91
Merge of that PR is still pending.

@cvuosalo
Copy link
Contributor

cvuosalo commented Nov 4, 2015

@smorovic: Yes, I see it now. I got confused because there are three 80X PRs that all include FastMonitoringService.cc. Please make sure there is no mix-up during merging that could cause the older version of the file to overwrite the correct version in #12245.

@cvuosalo
Copy link
Contributor

cvuosalo commented Nov 4, 2015

+1

For #12148 36f1757

fillDescriptions migration in DAQ modules. #12146 and #12147 are the 75X and 76X versions of this PR, and they have already been approved by Reco.

#12245 has a fixed version of FastMonitoringService.cc that should take precedence over the version of this file in this PR.

The code changes are satisfactory, and Jenkins tests against baseline CMSSW_8_0_X_2015-11-02-2300 show no significant differences.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 4, 2015

This pull request is fully signed and it will be integrated in one of the next CMSSW_8_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar

@smorovic
Copy link
Contributor Author

smorovic commented Nov 4, 2015

I was taking into account that these changes merge together (also git is smart to take into account "git mv" when it's merged with other commits). Usually also git doesn't silently revert changes, but produces conflicts. In my tests all three merges fine. If any conflict pops up, I will rebase as necessary.

@davidlange6
Copy link
Contributor

+1

cmsbuild added a commit that referenced this pull request Nov 5, 2015
fillDescriptions migration and parameter cleanup in DAQ modules (80X)
@cmsbuild cmsbuild merged commit b62b7f2 into cms-sw:CMSSW_8_0_X Nov 5, 2015
@smorovic smorovic deleted the fillDescriptions-80X branch November 13, 2015 10:49
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

6 participants