-
Notifications
You must be signed in to change notification settings - Fork 283
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
Score-P easyblock update (REVIEW) #889
Conversation
the previous code actually never worked as intended...
Automatic reply from Jenkins: Can I test this? |
Jenkins: ok to test |
@geimer Please ping me when you consider this done (or if you want me to take a close look & provide feedback). |
Easyblocks unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1890/console for more details). This pull request is now ready for review/testing. Please try and find someone who can tackle this; contact @boegel if you're not sure what to do. |
Easyblocks unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1893/console for more details). This pull request is now ready for review/testing. Please try and find someone who can tackle this; contact @boegel if you're not sure what to do. |
Easyblocks unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1898/console for more details). This pull request is now ready for review/testing. Please try and find someone who can tackle this; contact @boegel if you're not sure what to do. |
Easyblocks unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1950/console for more details). This pull request is now ready for review/testing. Please try and find someone who can tackle this; contact @boegel if you're not sure what to do. |
@geimer: I'm slowly closing the window for EasyBuild v2.8.0, PRs that don't make it in by the end of this week will have to wait until v2.9.0 (end of June'16); what's the status of this? I've already tagged it for v2.9.0 since it's still marked as WIP, but I can reconsider that if you want. |
Easyblocks unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1974/console for more details). This pull request is now ready for review/testing. Please try and find someone who can tackle this; contact @boegel if you're not sure what to do. |
@boegel: Now that |
Easyblocks unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-easyblocks-pr-builder/1978/console for more details). This pull request is now ready for review/testing. Please try and find someone who can tackle this; contact @boegel if you're not sure what to do. |
deps = { | ||
'binutils': ['--with-libbfd=%%s/%s' % get_software_libdir('binutils', fs=['libbfd.a'])], | ||
'binutils': ['--with-libbfd=%s'], |
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.
@geimer: the only change that raises a question for me is this one...
There must have been a good reason we're using get_software_libdir
here?
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.
@boegel: There was no good reason; it simply was a bug that went by unnoticed for quite some time. I've checked all Score-P releases back to v1.1 (Oct 2012) and the configure
option always worked like this:
--with-libbfd=(yes|no|<Path to libbfd installation>)
If you want to build with libbfd support but do not
have a libbfd in a standard location, you need to
explicitly specify the directory where it is
installed. On non-cross-compile systems we search
the system include and lib paths per default [yes];
on cross-compile systems, however, you have to
specify a path [no]. --with-libbfd is a shorthand
for --with-libbfd-include=<Path/include> and
--with-libbfd-lib=<Path/lib>. If these shorthand
assumptions are not correct, you can use the
explicit include and lib options directly.
--with-libbfd-include=<Path to libbfd headers>
--with-libbfd-lib=<Path to libbfd libraries>
That is, some absolute path of libbfd.a was passed in, the configure check tried to use it as a prefix for 'include' and 'lib', this obviously didn't help in making the compile/link check pass, and the compiler adapter fell back to using nm
instead of libbfd. configure
prints a big fat warning that the provided path does not exist, but it is so well hidden in the EB log file that basically nobody finds it...
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.
@geimer: That sounds very reasonable, but... We're missing something.
On top of current develop
, all builds using this Score_minus_P
easyblock work fine; that includes: Cube
(v4.x), LWM2
, OTF2
, Scalasca
and Score-P
itself.
On top of this updated easyblock however, two of the Score-P
builds fail (Score-P-1.2.1-gompi-1.4.12-no-OFED.eb
and Score-P-1.2.3-goolf-1.5.14.eb
), while another one works fine Score-P-1.4-foss-2015a.eb
... The other stuff using this easyblock works fine (Cube
(v4.x), LWM2
, OTF2
, Scalasca
).
So, it seems like we've introduced some kind of regression for Score-P 1.2.x?
Here's the relevant error message (reformatted for readability):
== 2016-05-13 18:58:24,165 build_log.py:152 ERROR cmd " ./configure --prefix=... --with-libbfd=$EBROOTBINUTILS " exited with exitcode 1 and output:
...
checking bfd.h usability... yes
checking bfd.h presence... yes
checking for bfd.h... yes
: error: cannot find libbfd.a, libbfd.so, or libbfd.dylib
configure: error: ./configure failed for build-mpi
Note that without this patch, it's not the absolute path of libbfd.a
being passed, but the path to the directory in which libbfd.a
resides, i.e. --with-libbfd=$EBROOTBINUTILS/lib
.
This apparently works with Score-P 1.2.x and 1.4 (despite it being wrong according to the help info for --with-libbfd
vs --with-libbfd-lib
.
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.
@geimer The original PR for the Score-P easyblock (#304) mentions the need for specifying the lib
subdir for --with-libbfd
(and --with-papi-lib
) rather than specifying the root, so it seems like we did have a good reason for this...
Could this have been a bug in the handling of --with-libbfd
option in Score 1.2.x, and this behaviour being retained as a fallback in later versions for backward compatibility?
Maybe @berndmohr remembers why we needed to specify the path to the /lib
subdirectory of the binutils
install directory rather than the root install prefix as suggested with the help output for --with-libbfd
?
My suggestion would be to put a version check in place for versions older than Score-P 1.4, and stick to using the /lib
approach for those, to avoid breaking stuff?
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.
tested last update, all good to go now, thanks @geimer! |
--with-libbfd
pathlibunwind
suppportSIONlib
supportAdd PGI compiler toolchain. easybuild-framework#1342)