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 support for %(sysroot)s template value #4359

Merged
merged 11 commits into from
Oct 26, 2023

Conversation

xinan1911
Copy link
Contributor

This improves sysroot support. For example this code line in wget can be generalized using sysroot template.
From
preconfigopts = "export PKG_CONFIG_PATH=/usr/lib64/pkgconfig:/usr/lib/pkgconfig:/usr/lib/x86_64-linux-gnu/pkgconfig && "
to
preconfigopts = "export PKG_CONFIG_PATH=%(sysroot)s/usr/lib64/pkgconfig:%(sysroot)s/usr/lib/pkgconfig:%(sysroot)s/usr/lib/x86_64-linux-gnu/pkgconfig && "

This works for both cases when --sysroot is passed and when it's not (i.e. when it defaults to None).

@casparvl casparvl added the EESSI Related to EESSI project label Oct 24, 2023
@easybuilders easybuilders deleted a comment from boegelbot Oct 25, 2023
@boegel boegel added this to the next release (4.8.2?) milestone Oct 25, 2023
@boegel
Copy link
Member

boegel commented Oct 25, 2023

I was looking into this too, here's the test I came up with to test the use of the %(sysroot)s template:

 def test_sysroot_template(self):
       """Test the %(sysroot)s template"""

       test_easyconfigs = os.path.join(os.path.abspath(os.path.dirname(__file__)), 'easyconfigs', 'test_ecs')
       toy_ec = os.path.join(test_easyconfigs, 't', 'toy', 'toy-0.0.eb')

       test_ec = os.path.join(self.test_prefix, 'test.eb')
       test_ec_txt = read_file(toy_ec)
       test_ec_txt += '\nconfigopts = "--sysroot=%(sysroot)s/"'
       write_file(test_ec, test_ec_txt)

       ec = EasyConfig(test_ec)
       self.assertEqual(ec['configopts'], "--sysroot=/")

       update_build_option('sysroot', self.test_prefix)

       ec = EasyConfig(test_ec)
       self.assertEqual(ec['configopts'], "--sysroot=%s/" % self.test_prefix)

@casparvl
Copy link
Contributor

Yep, update_build_option('sysroot', self.test_prefix) is the thing we needed in the test, and didn't know about.

I like that you captured it in one test, we'll update that. I also like how you just modified the toy.eb instead of setting the full text (we stole that from the PR you gave us as example, but it's not really needed here).

I do prefer our approach to using echo's, I find the configopts = "--sysroot=%(sysroot)s/" potentially confusing. It could be mistaken for trying to test if one can pass --sysroot as a configopt, whereas with the echo it's clearly about the template value.

I also think we should stick to setting it in the three pre*opts we did now (again, stolen from the PR you gave us as example), as it verifies that template value is set in all of these stages of (which are probably the most relevant ones in terms of where it might be used) and not just in the configure step.

I'll merge your approach into ours :)

@casparvl
Copy link
Contributor

Ah, wait, I only now see that you are actually not relying on running all steps, you just query the options again. Ok, then I understand why you did the suggestive --sysroot. I'll go with that, it saves some time in the CI since you don't have to actually run the steps in your case.

I'll still do it for configure build and install opts, just to validate that it works in all of those.

@casparvl
Copy link
Contributor

Hm, that's a completely different test. I guess the update_build_option carries over between tests, so we should probably put it back. I thought that build options were reset between tests anyway? Maybe not...

Caspar van Leeuwen added 4 commits October 25, 2023 14:02
…ts with clean env. The real problem was that the template_constant_dict has changed, so the expected output is now different - and should include sysroot and its value
@easybuilders easybuilders deleted a comment from boegelbot Oct 26, 2023
@boegel
Copy link
Member

boegel commented Oct 26, 2023

Hm, that's a completely different test. I guess the update_build_option carries over between tests, so we should probably put it back. I thought that build options were reset between tests anyway? Maybe not...

Hmm, if that's the case, then we should definitely fix that (in a separate PR), configuration options shouldn't carry over tests at all...

edit: Ah, you figured it out, some other tests were failing because the sysroot template was introduced, had nothing to do with carrying over of configuration options across tests, good

@easybuilders easybuilders deleted a comment from boegelbot Oct 26, 2023
Copy link
Member

@boegel boegel 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 Add sysroot template value add support for %(sysroot)s template value Oct 26, 2023
@boegel boegel merged commit bbfd3a7 into easybuilders:develop Oct 26, 2023
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EESSI Related to EESSI project enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants