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

Removing intel-specific sanity check for libdrm #9694

Merged

Conversation

edmondac
Copy link
Contributor

@edmondac edmondac commented Jan 16, 2020

(created using eb --new-pr)

This now requires easybuilders/easybuild-easyblocks#1983

@edmondac
Copy link
Contributor Author

On POWER9:

easyconfigs $ ls ../../../../EL7/EL7-power9/software/libdrm/2.4.99-GCCcore-8.3.0/lib/
libdrm_amdgpu.la    libdrm_amdgpu.so.1.0.0  libdrm_nouveau.so        libdrm_radeon.la    libdrm_radeon.so.1.0.1  libdrm.so.2.4.0  libkms.so.1
libdrm_amdgpu.so    libdrm.la               libdrm_nouveau.so.2      libdrm_radeon.so    libdrm.so               libkms.la        libkms.so.1.0.0
libdrm_amdgpu.so.1  libdrm_nouveau.la       libdrm_nouveau.so.2.0.0  libdrm_radeon.so.1  libdrm.so.2             libkms.so        pkgconfig

@edmondac
Copy link
Contributor Author

Test report by @bear-rsg
SUCCESS
Build succeeded for 30 out of 30 (2 easyconfigs in this PR)
bear-pg0305u03a.bear.cluster - Linux RHEL 7.6, 8335-GTX, Python 2.7.5
See https://gist.github.com/05f0b7804bebd8fd70084cd756596e7e for a full test report.

@edmondac
Copy link
Contributor Author

Test report by @bear-rsg
SUCCESS
Build succeeded for 30 out of 30 (2 easyconfigs in this PR)
bear-pg0211u03a.bear.cluster - Linux centos linux 7.7.1908, Intel(R) Xeon(R) Gold 6248 CPU @ 2.50GHz, Python 2.7.5
See https://gist.github.com/94cd89d0986d7332e2a171a3e0ad2e65 for a full test report.

@Flamefire
Copy link
Contributor

Test report by @Flamefire
SUCCESS
Build succeeded for 2 out of 2 (2 easyconfigs in this PR)
taurusml2 - Linux RHEL 7.6, 8335-GTX, Python 2.7.5
See https://gist.github.com/52517d021e7f0a1026bdbca5d76d2c38 for a full test report.

Flamefire
Flamefire previously approved these changes Feb 4, 2020
Copy link
Contributor

@Flamefire Flamefire left a comment

Choose a reason for hiding this comment

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

LGTM. Maybe we should implement arch-specific checks? On x86 we would want that library to be available

@boegel
Copy link
Member

boegel commented Feb 29, 2020

@Flamefire Making the sanity check arch-specific is easy through a custom easyblock, may be worth doing that here?

@boegel boegel added the change label Feb 29, 2020
@boegel boegel added this to the next release (4.1.2?) milestone Feb 29, 2020
@boegel
Copy link
Member

boegel commented Feb 29, 2020

Test report by @boegel
SUCCESS
Build succeeded for 2 out of 2 (2 easyconfigs in this PR)
generoso - Linux centos linux 7.6.1810, Intel(R) Xeon(R) CPU E5-2660 v4 @ 2.00GHz, Python 3.6.8
See https://gist.github.com/89d35158d6dde43b934d6c594d108d26 for a full test report.

@Flamefire
Copy link
Contributor

I was thinking about supporting that in the framework. We have arch specific EasyConfig settings already (checksum? filename? can't remember) so I'd rather allow 'files: [('x86', 'libdrm.so'), ...]

@boegel
Copy link
Member

boegel commented Feb 29, 2020

I was thinking about supporting that in the framework. We have arch specific EasyConfig settings already (checksum? filename? can't remember) so I'd rather allow 'files: [('x86', 'libdrm.so'), ...]

That won't work in that exact way though, because we already have support for tuples in sanity_check_paths to indicate alternatives (like ('lib/libfoo.a', 'lib64/libfoo.a').

@boegel
Copy link
Member

boegel commented Mar 1, 2020

@Flamefire @edmondac Is there an equivalent to libdrm_intel.so on POWER? If so, we can use the tuple format to add it as an alternative to libdrm_intel.so, something like:

sanity_check_paths = {
    'files': ['include/xf86drm.h', 'include/xf86drmMode.h',
              ('lib/libdrm_intel.%s' % SHLIB_EXT, 'lib/libdrm_power.%s' % SHLIB_EXT),

@branfosj
Copy link
Member

branfosj commented Mar 1, 2020

Seemingly there is no equivalent. Tested using libdrm 2.4.99 in 2019b.

On CascadeLake:

$ ls -1 ${EBROOTLIBDRM}/lib/
libdrm_amdgpu.la
libdrm_amdgpu.so
libdrm_amdgpu.so.1
libdrm_amdgpu.so.1.0.0
libdrm_intel.la
libdrm_intel.so
libdrm_intel.so.1
libdrm_intel.so.1.0.0
libdrm.la
libdrm_nouveau.la
libdrm_nouveau.so
libdrm_nouveau.so.2
libdrm_nouveau.so.2.0.0
libdrm_radeon.la
libdrm_radeon.so
libdrm_radeon.so.1
libdrm_radeon.so.1.0.1
libdrm.so
libdrm.so.2
libdrm.so.2.4.0
libkms.la
libkms.so
libkms.so.1
libkms.so.1.0.0

On POWER:

$ ls -1 ${EBROOTLIBDRM}/lib/  
libdrm_amdgpu.la
libdrm_amdgpu.so
libdrm_amdgpu.so.1
libdrm_amdgpu.so.1.0.0
libdrm.la
libdrm_nouveau.la
libdrm_nouveau.so
libdrm_nouveau.so.2
libdrm_nouveau.so.2.0.0
libdrm_radeon.la
libdrm_radeon.so
libdrm_radeon.so.1
libdrm_radeon.so.1.0.1
libdrm.so
libdrm.so.2
libdrm.so.2.4.0
libkms.la
libkms.so
libkms.so.1
libkms.so.1.0.0

@boegel
Copy link
Member

boegel commented Mar 1, 2020

Well, we could use lib/libdrm.so as a fallback?

@Flamefire
Copy link
Contributor

Well, we could use lib/libdrm.so as a fallback?

this is always there. So if you list this as an alternative you could as well not check for the other alternative

@boegel
Copy link
Member

boegel commented Mar 2, 2020

Test report by @boegel
SUCCESS
Build succeeded for 2 out of 2 (2 easyconfigs in this PR)
node3131.skitty.os - Linux centos linux 7.7.1908, Intel(R) Xeon(R) Gold 6140 CPU @ 2.30GHz, Python 3.6.8
See https://gist.github.com/0319421817dcc899addad6810e86702f for a full test report.

@boegel
Copy link
Member

boegel commented Mar 2, 2020

Test report by @boegel
SUCCESS
Build succeeded for 2 out of 2 (2 easyconfigs in this PR)
node2557.golett.os - Linux centos linux 7.7.1908, Intel(R) Xeon(R) CPU E5-2680 v3 @ 2.50GHz, Python 2.7.5
See https://gist.github.com/48a35e98ef75feb09c9e56b3944015b6 for a full test report.

@boegel
Copy link
Member

boegel commented Mar 2, 2020

OK, so we have a couple of options here:

  1. Just drop the check for libdrm_intel.so, which this PR currently does. I would rather not, but I don't care about it very strongly (I'm not sure we have a very specific reason to check for it). It's definitely the easy way out.

  2. Implement an easyblock for libdrm that only checks for libdrm_intel.so when building for x86. Shouldn't be too difficult either, and will clean up the libdrm easyconfigs a bit as added benefit.

  3. Come up with a more general approach in framework, for example adding support for specifying arch-specific stuff in sanity_check_paths, for example:

sanity_check_paths = {
    'files': [{'arch=x86_64': 'lib/libdrm_intel.%s' % SHLIB)EXT}, ...],
    'dirs': [...],
}

That shouldn't be extremely difficult either I think, and it may be worthwhile in the long run, but it's a bit more work I guess.

@Flamefire, @edmondac , @branfosj: pick your poison! :)

@Flamefire
Copy link
Contributor

My vote would be for 3). I guess one can use the logic implemented for dependencies?

@boegel
Copy link
Member

boegel commented Mar 2, 2020

My vote would be for 3). I guess one can use the logic implemented for dependencies?

Partially maybe, yes.

@edmondac
Copy link
Contributor Author

edmondac commented Mar 2, 2020

I was going to try option 2 @boegel and @Flamefire - but I don't have time to do option 3. @Flamefire are you up for doing option 3 yourself?

@Flamefire
Copy link
Contributor

I started working on it but gave up as it got incredibly complicate after encountering the type checking of EC options. The type of the values for the sanity_check_paths would need to be: "list of (string, tuple, dict of (string -> string | tuple))" and I don't feel like getting the type checking AND conversion implemented is worthwhile. IMO that code needs refactoring to only having to describe the type once, not as a definition and conversion. E.g. currently the description of the dependencies is wrong (not including the arch= handling) and the conversion incomplete which makes it succeed.

I'll keep my work here: https://github.com/Flamefire/easybuild-framework/tree/alternative_sanity_check_paths but for now I suggest an EasyBlock

@edmondac
Copy link
Contributor Author

edmondac commented Mar 4, 2020

OK - I'll work on option 2 :-)

@edmondac
Copy link
Contributor Author

edmondac commented Mar 4, 2020

Changed libdrm to use the custom easyblock in easybuilders/easybuild-easyblocks#1983

@Flamefire
Copy link
Contributor

Test report by @Flamefire
SUCCESS
Build succeeded for 2 out of 2 (2 easyconfigs in this PR)
taurusi6072.taurus.hrsk.tu-dresden.de - Linux RHEL 7.7, Intel(R) Xeon(R) CPU E5-2680 v3 @ 2.50GHz, Python 2.7.5
See https://gist.github.com/8550f2f5eabb42787b4016c049ab696d for a full test report.

@edmondac
Copy link
Contributor Author

edmondac commented Mar 4, 2020

I've got test reports building now, but they're making GCC 8.2.0 currently...

@Flamefire
Copy link
Contributor

Test report by @Flamefire
SUCCESS
Build succeeded for 2 out of 2 (2 easyconfigs in this PR)
taurusml11 - Linux RHEL 7.6, 8335-GTX, Python 2.7.5
See https://gist.github.com/b45748e34d338764d72fc6d4235918d4 for a full test report.

Copy link
Contributor

@Flamefire Flamefire left a comment

Choose a reason for hiding this comment

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

Working on x86 and Power9

@edmondac
Copy link
Contributor Author

edmondac commented Mar 4, 2020

Test report by @bear-rsg
SUCCESS
Build succeeded for 31 out of 31 (2 easyconfigs in this PR)
bear-pg0306u03a.bear.cluster - Linux RHEL 7.6, 8335-GTX, Python 2.7.5
See https://gist.github.com/b0f52dff5e01217e2e218d4e27c2b006 for a full test report.

@edmondac
Copy link
Contributor Author

edmondac commented Mar 4, 2020

Test report by @bear-rsg
SUCCESS
Build succeeded for 31 out of 31 (2 easyconfigs in this PR)
bear-pg0211u03a.bear.cluster - Linux centos linux 7.7.1908, Intel(R) Xeon(R) Gold 6248 CPU @ 2.50GHz, Python 2.7.5
See https://gist.github.com/445d6dba1d199b3a2b56c7552e568417 for a full test report.

@boegel
Copy link
Member

boegel commented Mar 6, 2020

Test report by @boegel
SUCCESS
Build succeeded for 2 out of 2 (2 easyconfigs in this PR)
node3152.skitty.os - Linux centos linux 7.7.1908, Intel(R) Xeon(R) Gold 6140 CPU @ 2.30GHz, Python 3.6.8
See https://gist.github.com/e9747078927dde05307207ba575b5017 for a full test report.

@boegel
Copy link
Member

boegel commented Mar 6, 2020

Test report by @boegel
SUCCESS
Build succeeded for 2 out of 2 (2 easyconfigs in this PR)
generoso - Linux centos linux 7.6.1810, Intel(R) Xeon(R) CPU E5-2660 v4 @ 2.00GHz, Python 3.6.8
See https://gist.github.com/527669c0892e65d68921c77ce6c22197 for a full test report.

@boegel
Copy link
Member

boegel commented Mar 6, 2020

Test report by @boegel
SUCCESS
Build succeeded for 2 out of 2 (2 easyconfigs in this PR)
node2412.golett.os - Linux centos linux 7.7.1908, Intel(R) Xeon(R) CPU E5-2680 v3 @ 2.50GHz, Python 2.7.5
See https://gist.github.com/accb62d3dfadd9166a852ddee74c7249 for a full test report.

@boegel
Copy link
Member

boegel commented Mar 6, 2020

re-triggered tests now easybuilders/easybuild-easyblocks#1983 is merged...

@easybuilders easybuilders deleted a comment from boegelbot Mar 6, 2020
@boegel
Copy link
Member

boegel commented Mar 6, 2020

@edmondac You'll need to run eb --sync-pr-with-develop 9694 for this, since re-triggering the tests isn't working (because the changes from #9925 are missing in your branch)

@edmondac edmondac closed this Mar 6, 2020
@edmondac edmondac reopened this Mar 6, 2020
@edmondac edmondac requested a review from boegel March 6, 2020 16:21
@boegel boegel added the bug fix label Mar 7, 2020
@boegel
Copy link
Member

boegel commented Mar 7, 2020

Test report by @boegel
SUCCESS
Build succeeded for 2 out of 2 (2 easyconfigs in this PR)
generoso - Linux centos linux 7.6.1810, Intel(R) Xeon(R) CPU E5-2660 v4 @ 2.00GHz, Python 3.6.8
See https://gist.github.com/1c30929819da8dd0c41a5c74b16dd8ea for a full test report.

@boegel
Copy link
Member

boegel commented Mar 7, 2020

Going in, thanks @edmondac!

@boegel boegel merged commit e53613e into easybuilders:develop Mar 7, 2020
@edmondac edmondac deleted the 20200116163226_new_pr_libdrm2499 branch March 9, 2020 10:27
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.

4 participants