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

update GCC easyblock to ensure that --sysroot is passed to linker (but only when it needs to be) #2921

Merged
merged 3 commits into from
May 27, 2023

Conversation

trz42
Copy link
Contributor

@trz42 trz42 commented Apr 21, 2023

This removes an adjustment to gcc/gcc.c which avoided that --sysroot is passed to the linker. Since a recent change to how Gentoo supports sysroot (see gentoo/gentoo#28851) we need to change the easyblock for gcc.

A bit of testing revealed that only the removed line(s) are not needed any longer.

So far it has been tested for an EESSI-like environment with a recent compatibility layer version (Gentoo commit from April 17) with:

  • GCC/9.3.0 not tested, but might be ignore for EESSI at least
  • GCC/10.3.0 on x86_64/{generic,amd/zen2,intel/broadwell,intel/cascadelake,intel/skylake_avx512 and aarch64/generic
  • GCC/11.3.0 on x86_64/{generic,amd/zen2,intel/broadwell,intel/cascadelake,intel/skylake_avx512 and aarch64/generic
  • GCC/12.2.0 on x86_64/{generic,amd/zen2,intel/broadwell,intel/cascadelake,intel/skylake_avx512 and aarch64/generic

@bedroge
Copy link
Contributor

bedroge commented Apr 22, 2023

In order to not break older installations that still use the old approach, we would have to add some check(s) here. We can perhaps do that by inspecting the file $EPREFIX/usr/lib64/libc.so, and find out if the paths in GROUP( .... ) are prefixed by the value of sysroot?

@boegel boegel added the EESSI Related to EESSI project label Apr 22, 2023
@boegel boegel added this to the next release (4.7.2) milestone Apr 22, 2023
@boegel
Copy link
Member

boegel commented Apr 22, 2023

Yeah, we should figure out a way to opt-out of patching out the --sysroot from gcc.c conditionally, since it is required for older EESSI versions.

What was changed exactly that no longer required this, do we have references for this?

cc @bartoldeman, who no doubt can provide some guided info here...

@boegel boegel added the bug fix label Apr 22, 2023
@ocaisa
Copy link
Member

ocaisa commented Apr 22, 2023

I think we just add an easyconfig switch here to turn this off (defaulting to on), and a hook in eessi to trigger it. In the next major release we change the default

@bedroge
Copy link
Contributor

bedroge commented Apr 22, 2023

Yeah, we should figure out a way to opt-out of patching out the --sysroot from gcc.c conditionally, since it is required for older EESSI versions.

What was changed exactly that no longer required this, do we have references for this?

cc @bartoldeman, who no doubt can provide some guided info here...

See: gentoo/gentoo#28851 and https://www.gentoo.org/support/news-items/2023-01-28-rap-prefix-sysroot.html.

@bedroge
Copy link
Contributor

bedroge commented Apr 22, 2023

In order to not break older installations that still use the old approach, we would have to add some check(s) here. We can perhaps do that by inspecting the file $EPREFIX/usr/lib64/libc.so, and find out if the paths in GROUP( .... ) are prefixed by the value of sysroot?

The comment at https://github.com/gentoo/gentoo/pull/28851/files#diff-bbba0e850dfb18cee4bb09d6c00d235e935e24baa2199ce82a628d84dd16cb58R1317 shows the difference between Prefix installations with the old and new approach:

	# Before: GROUP ( /foo/lib64/libc.so.6 /foo/usr/lib64/libc_nonshared.a  AS_NEEDED ( /foo/lib64/ld-linux-x86-64.so.2 ) )
	# After: GROUP ( /lib64/libc.so.6 /usr/lib64/libc_nonshared.a  AS_NEEDED ( /lib64/ld-linux-x86-64.so.2 ) )

So we may be able to check those paths and determine if they are "prefixed" (with the same prefix as specified with --sysroot) or not.

@boegel boegel changed the title ensure that --sysroot is passed to linker update GCC easyblock to ensure that --sysroot is passed to linker May 4, 2023
@bartoldeman
Copy link
Contributor

Yes @bedroge is correct here.
There has been another change: gcc.c was renamed to gcc.cc, so if want to be 100% compatible we have to account for that as well. I'll try and write something and get back tomorrow morning

Check the contents of libc.so: only if the entries are prefixed, patch
gcc.c (or gcc.cc for GCC 12+), to account for the changes in
gentoo/gentoo#28851
@bartoldeman
Copy link
Contributor

@trz42
See trz42#1 for my attempt. I verified it still does the patching in our older sysroot-ed Gentoo installation.

Patch gcc.c* only for older sysroot-ed installations.
@boegel boegel changed the title update GCC easyblock to ensure that --sysroot is passed to linker update GCC easyblock to ensure that --sysroot is passed to linker (but only when it needs to be) May 27, 2023
boegel added a commit to SebastianAchilles/easybuild-easyblocks that referenced this pull request May 27, 2023
@boegel
Copy link
Member

boegel commented May 27, 2023

This has been tested extensively in EESSI, cfr. EESSI/software-layer#250

Only has impact when EasyBuild is configured with an alternate sysroot.

I'll go ahead and merge this to be included last-minute with EasyBuild v4.7.2, test reports are coming up to confirm this doesn't cause trouble.

Thanks for the effort on this @trz42 and @bartoldeman!

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 merged commit 7a1730c into easybuilders:develop May 27, 2023
@boegel
Copy link
Member

boegel commented May 27, 2023

Test report by @boegel

Overview of tested easyconfigs (in order)

  • SUCCESS GCCcore-9.3.0.eb
  • SUCCESS GCCcore-10.3.0.eb
  • SUCCESS GCCcore-11.3.0.eb
  • SUCCESS GCCcore-12.2.0.eb
  • SUCCESS GCCcore-13.1.0.eb

Build succeeded for 5 out of 5 (5 easyconfigs in total)
node5414.dodrio.os - Linux RHEL 8.6, x86_64, AMD EPYC 7763 64-Core Processor (zen3), Python 3.6.8
See https://gist.github.com/boegel/08d674edb228f44ee06bd6cf490becf2 for a full test report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix EESSI Related to EESSI project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants