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

Add mkFit as an external #5150

Merged
merged 2 commits into from
Aug 23, 2019
Merged

Conversation

makortel
Copy link
Contributor

This PR adds the mkFit as an external package (the CMSSW side will follow).

Pardon my ignorance, but will the contents of a PR to gcc700 propagate automatically to other compiler branches?

@slava77

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @makortel (Matti Kortelainen) for branch IB/CMSSW_11_0_X/gcc700.

@cmsbuild, @smuzaffar, @gudrutis, @mrodozov can you please review it and eventually sign? Thanks.
cms-bot commands are listed here

@makortel
Copy link
Contributor Author

@cmsbuild, please test

(am I powerful enough also here?)

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 13, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2009/console Started: 2019/08/13 23:38

mkfit.spec Outdated Show resolved Hide resolved
@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8d5419/2009/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2939508
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2939165
  • DQMHistoTests: Total skipped: 341
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 142 log files, 14 edm output root files, 34 DQM output files

@davidlange6
Copy link
Contributor

davidlange6 commented Aug 14, 2019 via email

@slava77
Copy link
Contributor

slava77 commented Aug 14, 2019

No. That gets defined elsewhere (eg gcc.spec)

please point me to the line where it's done.
I recall that in some other externals we had to set some flags manually (e.g. in TF and mxnet).

@davidlange6
Copy link
Contributor

davidlange6 commented Aug 14, 2019 via email

@slava77
Copy link
Contributor

slava77 commented Aug 14, 2019

I meant compiler (gcc) and a specific target architecture (core2 or sse3 or avx etc)

@davidlange6
Copy link
Contributor

davidlange6 commented Aug 14, 2019 via email

@slava77
Copy link
Contributor

slava77 commented Aug 14, 2019

how does the other spec file know about the fanciness of the mkFit/Makefile.config ?
About the arch, it looks like -mavx is the default.

@davidlange6
Copy link
Contributor

davidlange6 commented Aug 14, 2019 via email

@slava77
Copy link
Contributor

slava77 commented Aug 14, 2019

no other spec file should need to know about some internal makefile of mkFit

I do not quite understand what you mean.
My comment was about a line in mkfit.spec "make" call to add more than just TBB_PREFIX=$TBB_ROOT to the build command.

I'm so far reading your comments that all arguments specifying which make rule/setting to check is defined in the gcc.spec file and at the same time in no other file except mkfit.spec.

@davidlange6
Copy link
Contributor

davidlange6 commented Aug 14, 2019 via email

@slava77
Copy link
Contributor

slava77 commented Aug 14, 2019

maybe its worth having a look at some other spec files to see the typical interface (eg, root.spec) You'll see the way things like passing in the location of dependencies (gcc, TBB_ROOT) are typically parsed by software packages.

Just to be clear, I'm making comments in the context that this is an external and this PR can not modify the files in the mkFit repository.
This is our standard approach in most of the cases of externals that I'm aware of.

@makortel
Copy link
Contributor Author

anyway if adding VEC_GCC="-march=core2" to the make command line does whats needed in the mkfit build rules, then you should add that to mkfit.spec.

I'm working on that (thanks for pointing it out @slava77), update should come shortly.

@cmsbuild
Copy link
Contributor

Pull request #5150 was updated.

@makortel
Copy link
Contributor Author

@cmsbuild, please test

Added VEC_GCC="-march=core2" as suggested above.

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 14, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2023/console Started: 2019/08/14 16:51

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8d5419/2023/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2939508
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2939166
  • DQMHistoTests: Total skipped: 341
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 142 log files, 14 edm output root files, 34 DQM output files

@slava77
Copy link
Contributor

slava77 commented Aug 16, 2019

@makortel
thanks for the update.
I see that the standard tests passed.

@smuzaffar @mrodozov
please consider merging this soon.
Thank you.

@slava77
Copy link
Contributor

slava77 commented Aug 20, 2019

@smuzaffar @mrodozov
please clarify on the status of the review of this external.
Thank you.

@slava77
Copy link
Contributor

slava77 commented Aug 21, 2019

@smuzaffar @mrodozov
please clarify on the status of the review of this external.
Thank you.

ping

@mrodozov
Copy link
Contributor

+externals
I can merge this in the evening

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next IB/CMSSW_11_0_X/gcc700 IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

@mrodozov mrodozov merged commit 1c99e49 into cms-sw:IB/CMSSW_11_0_X/gcc700 Aug 23, 2019
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.

5 participants