Skip to content

take into account user defined gallium_drivers and swr_arches in Mesa easyblock #2006

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

Merged
merged 8 commits into from
Apr 1, 2020

Conversation

lexming
Copy link
Contributor

@lexming lexming commented Mar 29, 2020

Current mesa easyblock fails the sanity checks if user customizes gallium-drivers or swr-arches in configopts.

This PR fixes that issue by

  1. first retrieve the values for those options from configopts
  2. gallium-drivers is only set by easyblock if missing from configopts (as before)
  3. swr-arches is only set by easyblock if missing from configopts and swr is present in gallium-drivers
  4. SWR libs are added to sanity checks depending on swr-arches and regardless of who defined swr-arches

Tested cases:

  • default easyconfig of Mesa-20.0.2-GCCcore-9.3.0.eb
    • easyblock behaves as before this PR
  • adding configopts += "-Dgallium-drivers=''"
    • the build will fail as at least one driver is missing, but easyblock will respect this configuration if it's set by user
  • adding configopts += "-Dgallium-drivers='swrast'"
    • no swr-arches added and sanity checks do not include any SWR libs
  • adding configopts += "-Dgallium-drivers='swrast,swr'"
    • swr-arches added depending on CPU arch and respective SWR libs checked in sanity checks
  • adding configopts += "-Dgallium-drivers='swrast,swr' -Dswr-arches=avx2"
    • swr-arches is not modified and libswrAVX2.so is added to sanity checks

@boegel boegel added this to the next release (4.2.0) milestone Mar 29, 2020
@Flamefire
Copy link
Contributor

Flamefire commented Mar 30, 2020

I haven't seen this one when I opened #2007 but it is worth comparing them. What #2007 does better:

  • Use the last option from configopts when multiple are present
  • Handles the case when swr-arches was passed but swr is not build. I expect this PR to fail in this case.
  • Check added to sanity_check_* (where it actually belongs)

I don't think self.gallium_configopts is actually required. (Same actually for #2007 and self.swr_arches) as IMO setting the config params should be done in the config step to make it easier to understand.

@lexming
Copy link
Contributor Author

lexming commented Mar 30, 2020

@Flamefire you are right, this easyblock would have failed with a configopts such as "-Dgallium-drivers='swrast' -Dswr-arches=avx2". I've checked your PR and made the following changes:

  • swr-arches configuration and sanity checks only considered if swr is in gallium-drivers
  • function get_configopt_value() always takes last provided value and warns if multiple options were found
  • added additional log messages

I tested these changes on Mesa-20.0.2-GCCcore-9.3.0.eb with the configration options listed in the description of #2006 (comment), plus the following ones:

  • configopts += "-Dgallium-drivers='swrast' -Dswr-arches=avx"
    • SWR is not built and no sanity checks are added for SWR libs
  • configopts += "-Dswr-arches=avx"
    • SWR will be built and SWR libs checked depending on the architecture
  • configopts += "-Dgallium-drivers='swrast,swr' -Dswr-arches=avx -Dgallium-drivers='swrast,swr' -Dswr-arches=avx2"
    • only last option is taken into account

Regarding the use of self.gallium_configopts, it is true that all code sitting in __init__() could be moved to configure_step() and self.gallium_configopts would not be necessary. I do not really mind which option is used, both have their advantages and disadvantages. I'll let this decision to @boegel .

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.

From @boegel

doing stuff in the constructor can be important to ensure --module-only works as expected (since that skips configure_step, but doesn't skip sanity_check_step)

So this is better. Still some minor things.

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!

@boegel boegel changed the title mesa: take into account user defined gallium_drivers and swr_arches take into account user defined gallium_drivers and swr_arches in Mesa easyblock Apr 1, 2020
@boegel
Copy link
Member

boegel commented Apr 1, 2020

re-reviewed, tested and approved, thanks @lexming and @Flamefire!

@boegel boegel merged commit af41c47 into easybuilders:develop Apr 1, 2020
@lexming lexming deleted the mesa branch April 1, 2020 17:38
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.

3 participants