sys-devel/sysroot-wrappers: Drop in favour of setting CPP/CC/CXX #2177
Conversation
Signed-off-by: James Le Cuirot <jlecuirot@microsoft.com>
Signed-off-by: James Le Cuirot <jlecuirot@microsoft.com>
Signed-off-by: James Le Cuirot <jlecuirot@microsoft.com>
cross-boss has shown that setting CPP/CC/CXX is a highly effective alternative to using wrappers. This approach is not susceptible to the PATH or SYSROOT variables being lost. It works so well because Gentoo uses a similar approach for amd64 multilib and failure to respect these variables is treated as a serious QA violation. On the downside, it can lead to the --sysroot argument leaking into certain files and subsequently being applied to builds started from the target environment. This can be worked around with a symlink, but it is tidier to strip such flags after the build. cross-boss does this with a pkg_preinst hook that carefully strips them from non-binary files. This hook has been adapted here, but only for the dev profile because we don't build the SDK with sysroot flags and we don't install build tools to the production image. It has been tested against binary packages. There has been talk in Gentoo circles of achieving this using gcc specs instead, which would not suffer from the downside, but my initial investigation has found that you cannot simply add --sysroot this way, and it may not be viable at all. Signed-off-by: James Le Cuirot <jlecuirot@microsoft.com>
| local FILES FILE | ||
|
|
||
| # Gather all the non-binary files with the --sysroot or --with-sysroot flag. | ||
| mapfile -t -d '' < <(find "${D}" -type f -exec grep -lZEIe "--sysroot=${ROOT}\b|--with-sysroot=${EROOT}\b" {} +) FILES |
There was a problem hiding this comment.
I'm uneasy seeing some variables passed to grep as a regexp. I hope we won't see some weird paths in ${ROOT} or ${EROOT}. Maybe we could escape those paths somehow? Ideally, we could pass -F instead of -E to grep, but then \b won't work…
There was a problem hiding this comment.
I'm also curious if there are files that quote (either with single quotes or double quotes) the value of the flag (--sysroot="/quoted/path").
There was a problem hiding this comment.
I'll see whether I can use fixed strings, although these values are only ever going to be things like /build/arm64-usr.
We control the --sysroot flag, so it will never have quotes. Similarly, the --with-sysroot flag comes from Portage itself.
There was a problem hiding this comment.
I think you could use fixed strings. There is a "risk" that this would pick up more files (with --sysroot=/build/amd64-usr/foo out of the blue), but your sed invocation should be careful enough to ignore the non-matching lines.
sdk_container/src/third_party/coreos-overlay/profiles/coreos/targets/generic/dev/profile.bashrc
Show resolved
Hide resolved
That's fair. I'd forgotten about this stripping when I initially proposed the idea. Then I figured it didn't apply to Flatcar. Then I realised that it did, but only to the dev image, which isn't so bad. I looked further into the specs solution at that point, but no luck there. For what it's worth, I've never seen any fall out from this. Very few text files have these flags baked in. The Python "sysconfigdata" file is one example. If you'd prefer to have a |
|
Build action triggered: https://github.com/flatcar/scripts/actions/runs/10300334540 |
| # Carefully strip the sysroot flags. | ||
| local sedargs=( -i -r ) | ||
| local flag | ||
| for flag in --{,with-}sysroot="${EROOT}"; do |
There was a problem hiding this comment.
Should this be the following instead? To match the flags you check with grep?
| for flag in --{,with-}sysroot="${EROOT}"; do | |
| for flag in "--sysroot=${ROOT}" "--with-sysroot=${EROOT}"; do |
There was a problem hiding this comment.
I think ROOT and EROOT are basically the same thing in our case, because we don't use eprefix, but still.
There was a problem hiding this comment.
Well spotted. This line is different in cross-boss, so it isn't an issue there. It's not really an issue for Flatcar either because we don't use prefix here, but still!
There was a problem hiding this comment.
I thought about this some more and realised both should be ESYSROOT to match what we set the flag too. I guess I just forgot to update this in cross-boss way back. It's academic most of the time, but it does make a difference in this case.
There was a problem hiding this comment.
It's funny how Flatcar is forcing me to rethink things I've been doing fine for years. I'd forgotten that I didn't use ESYSROOT here because it's not defined in the pkg phases. I'm not sure whether the dev image is prepared in the same ROOT location as the binary packages it's built from, but I'd rather assume it's not. That means I need to change this hook from pre_pkg_preinst to post_src_install, and that means it needs to apply to the generic profile, not just the dev profile. While I'm happy to make this change for cross-boss, I'm now leaning towards just making a symlink for Flatcar. What do you think?
There was a problem hiding this comment.
Yes, the dev container image. I know they are both built in a ROOT, but I'm not sure whether that location is exactly the same for both images. Probably not. If not, then ESYSROOT will differ from EROOT, and the stripping therefore won't work if only applied when installing the binary packages.
The symlink wouldn't be in the binary packages, so it wouldn't appear in the production image. It would just be added to the dev container image before finalising it.
I think setting these variables and adding a single symlink only to the dev container image is probably still cleaner on balance, but I admit that I have been wavering here. I would like it to be cleaner, not just for Flatcar, but for cross-boss as well, so this has inspired me to look further into alternatives.
The specs-based solution isn't viable without changes to gcc, which might be hard sell to upstream, so I came up with a much simpler gcc patch (about 5 new lines in one place) that doesn't use specs at all. With that, things literally just work with no special flags or other adjustments needed. It's not like Gentoo doesn't patch gcc already. I'm running it past the Gentoo toolchain folks, and I'm currently testing out a full @system build with cross-boss.
There was a problem hiding this comment.
Yes, the dev container image. I know they are both built in a ROOT, but I'm not sure whether that location is exactly the same for both images. Probably not.
It happens to be the same ROOT for both, and they are built sequentially but we shouldn't depend on that.
If not, then ESYSROOT will differ from EROOT, and the stripping therefore won't work if only applied when installing the binary packages.
I don't follow this conclusion. At a given time ESYSROOT should be equal to EROOT, from man emerge:
SYSROOT = [path]
(...) The value must either be / or equal to ROOT. When cross-compiling, only the latter is valid.
Unless you mean that the value of ROOT will be different between binpkg generation and installation.
The symlink wouldn't be in the binary packages, so it wouldn't appear in the production image. It would just be added to the dev container image before finalising it.
Oh, sorry I didn't clarify. To me the dev container image is a production artifact that is shipped to users.
The specs-based solution isn't viable without changes to gcc, which might be hard sell to upstream, so I came up with a much simpler gcc patch (about 5 new lines in one place) that doesn't use specs at all.
What does the gcc patch look like?
There was a problem hiding this comment.
I don't follow this conclusion. At a given time ESYSROOT should be equal to EROOT, from man emerge:
I wrote that line. 😀 It actually doesn't work for three reasons. As you say, ROOT can change between building and installation. Also, Portage doesn't set ESYSROOT during the pkg phases, and it doesn't preserve its value in binary packages.
Here's the gcc patch. I'll probably add something to the man page too.
diff -Naur a/gcc/gcc.cc b/gcc/gcc.cc
--- a/gcc/gcc.cc 2024-08-01 23:34:33.525082176 +0100
+++ b/gcc/gcc.cc 2024-08-01 23:43:31.557156041 +0100
@@ -5527,6 +5527,16 @@
"BINUTILS", PREFIX_PRIORITY_LAST, 0, 1);
free (tooldir_prefix);
+ if (*cross_compile == '1' && !target_system_root_changed)
+ {
+ const char *esysroot = env.get("ESYSROOT");
+ if (esysroot && esysroot[0] != '\0' && strcmp(esysroot, "/") != 0 && (!target_system_root || strcmp(esysroot, target_system_root) != 0))
+ {
+ target_system_root = esysroot;
+ target_system_root_changed = 1;
+ }
+ }
+
#if defined(TARGET_SYSTEM_ROOT_RELOCATABLE) && !defined(VMS)
/* If the normal TARGET_SYSTEM_ROOT is inside of $exec_prefix,
then consider it to relocate with the rest of the GCC installationMy @system build successfully completed. There were no traces of the sysroot flag.
There was a problem hiding this comment.
i see, that's basically the sysroot-wrappers approach but baked into gcc
There was a problem hiding this comment.
It's got the initial thumbs up from Sam at Gentoo. I'm now doing a two-phase Jenkins build with it to really put it through its paces. If that works, I'll press ahead and open a new PR to supersede this one.
Nah, I'm fine with what you did here. I have no clue about Python sysconfigdata file, so no idea if the baked-in sysroot there is harmless or not. |
|
Closing in favour of #2207. |
Drop sysroot-wrappers in favour of setting CPP/CC/CXX
cross-boss has shown that setting CPP/CC/CXX is a highly effective alternative to using wrappers. This approach is not susceptible to the PATH or SYSROOT variables being lost. It works so well because Gentoo uses a similar approach for amd64 multilib and failure to respect these variables is treated as a serious QA violation.
On the downside, it can lead to the
--sysrootargument leaking into certain files and subsequently being applied to builds started from the target environment. This can be worked around with a symlink, but it is tidier to strip such flags after the build. cross-boss does this with apkg_preinsthook that carefully strips them from non-binary files. This hook has been adapted here, but only for the dev profile because we don't build the SDK with sysroot flags and we don't install build tools to the production image. It has been tested against binary packages.There has been talk in Gentoo circles of achieving this using gcc specs instead, which would not suffer from the downside, but my initial investigation has found that you cannot simply add
--sysrootthis way, and it may not be viable at all.This change allows us to use Gentoo's own dev-lang/rust package, which currently fails with the sysroot-wrappers. It requires some eclass changes for other Rust packages though. These have already been submitted upstream and I will apply them before merging this.
How to use
You can perform a full build and run the tests, but I have already done it.
Testing done
A two-phase Jenkins build was successful. This was before I realised the flag stripping is needed for the development image though, so I've added that and am now running it again.
changelog/directory (user-facing change, bug fix, security fix, update) -- N/A/bootand/usrsize, packages, list files for any missing binaries, kernel modules, config files, kernel modules, etc.