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
Update Dyninst package file for TBB dependency #9728
Conversation
Dyninst master and the soon to be released version requires the tbb package. This mod updates the tbb versions adding the one that dyninst uses and adds the required spack package changes to dyninst/package.py.
Can someone help me decipher what failed in the tests? Looks like a timeout w.r.t. the test. |
@@ -18,6 +18,7 @@ class IntelTbb(Package): | |||
homepage = "http://www.threadingbuildingblocks.org/" | |||
|
|||
# See url_for_version() below. | |||
version('2018.6', '9a0f78db4f72356068b00f29f54ee6bc') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to consider moving this version below the 2019 version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lee218llnl I just submitted the change. I was debugging whether it would be selected if listed on top and forgot to put it back (after the 2019 entry).
@lee218llnl Greg - do you know if the failures are test related? I would guess the code in this change would not cause anything to fail. |
@@ -40,6 +40,7 @@ class Dyninst(Package): | |||
depends_on("libdwarf", when='@:9') | |||
depends_on("boost@1.42:") | |||
depends_on('libiberty+pic') | |||
depends_on("tbb@2018.6:") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you may want to add when='@develop' for the tbb dependency, since older versions do not depend on tbb.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lee218llnl Good point. I will change that.
I am not familiar with the test infrastructure, but I agree that it does not look related to your changes. |
@@ -73,6 +74,10 @@ def install(self, spec, prefix): | |||
# For @develop + use elfutils libdw, libelf is an abstraction | |||
# we are really using elfutils here | |||
if spec.satisfies('@develop'): | |||
tbb = spec['tbb'].prefix | |||
args.append('-DTBB_INCLUDE_DIRS=%s' % tbb.include) | |||
args.append('-DTBB_LIBRARIES=%s' % join_path( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is very picky, but it looks like you are one space off from lining up your '%' characters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lee218llnl I fixed that. Will be in the next checkin
Dyninst master and the soon to be released
version requires the tbb package. This mod
updates the tbb versions adding the one that
dyninst uses and adds the required spack package
changes to dyninst/package.py.