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

remove ref to testCondDBDict which is not part of this package any more #11628

Merged
merged 2 commits into from Oct 5, 2015
Merged

remove ref to testCondDBDict which is not part of this package any more #11628

merged 2 commits into from Oct 5, 2015

Conversation

smuzaffar
Copy link
Contributor

No description provided.

@smuzaffar
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 5, 2015

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

remove ref to testCondDBDict which is not part of this package any more

It involves the following packages:

CondCore/CondDB

@ggovi, @cmsbuild can you please review it and eventually sign? Thanks.
@ghellwig, @apfeiffer1 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.

@davidlt
Copy link
Contributor

davidlt commented Oct 5, 2015

I commented on this Saturday. testRootStreaming needs the dictionary, otherwise it will fail. The dictionary files and selections files are kept, but for some reason building dictionary was disabled. (See #11120).

It was removed to silence some warning from SCRAM: apfeiffer1@c7b8f3e#diff-156022378736cc699d979c4a9edf1664

@smuzaffar
Copy link
Contributor Author

@davidlt, ah ok, yes testRootStreaming need the MyTestData dictionary in order to run. I think just added a empty cc file and generating a library in this test directory should allow to generate the missing dictionary

@davidlt
Copy link
Contributor

davidlt commented Oct 5, 2015

There is all the files need for genreflex command. That generates a C++ file for dictionary, aren't that enough?

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 5, 2015

Pull request #11628 was updated. @ggovi, @cmsbuild can you please check and sign again.

@smuzaffar
Copy link
Contributor Author

yes but one needs to explicitly tell scram that it should also generate a library along with other products on the test directory. SCRAM does it automatically for src sub-directory but of all the other sub-dirs one needs to explicitly ask for build products. I have updated the PR where I have just updated the test/BuildFile.xml so that it can generate a share library (magic part is that here it doe snot need a cc file)

@smuzaffar
Copy link
Contributor Author

please test

@@ -17,11 +18,9 @@
</bin>

<bin file="testRootStreaming.cpp" name="testRootStreaming">
<lib name="testCondDBDict"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Does SCRAM automatically link the dictionary to everything, or we need to bring back this statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think root is going to take care of it by reading the testCondDBDict_xr.rootmap file ...right? at least unit tests run locally without explicitly linking to testCondDBDict

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 5, 2015

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 5, 2015

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 5, 2015

@ggovi
Copy link
Contributor

ggovi commented Oct 5, 2015

+1

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 5, 2015

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

@davidlange6
Copy link
Contributor

+1

cmsbuild added a commit that referenced this pull request Oct 5, 2015
…2300

remove ref to testCondDBDict which is not part of this package any more
@cmsbuild cmsbuild merged commit c855f26 into cms-sw:CMSSW_7_6_X Oct 5, 2015
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