-
Notifications
You must be signed in to change notification settings - Fork 50
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
sys-devel/sysroot-wrappers: Drop in favour of setting CPP/CC/CXX #2177
Changes from all commits
faf09c4
0dd8618
9773eae
0b23ff0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,29 @@ | ||||||
# Adapted from cross-boss. Only needed 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. | ||||||
cros_pre_pkg_preinst_strip_sysroot() { | ||||||
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 | ||||||
|
||||||
# Only continue if there are any. | ||||||
[[ ${#FILES[@]} -eq 0 ]] && return | ||||||
|
||||||
einfo "Stripping sysroot flags from:" | ||||||
for FILE in "${FILES[@]}"; do | ||||||
einfo " - ${FILE#${D}}" | ||||||
done | ||||||
chewi marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
# 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be the following instead? To match the flags you check with
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about this some more and realised both should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the dev container image. I know they are both built in a 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It happens to be the same ROOT for both, and they are built sequentially but we shouldn't depend on that.
I don't follow this conclusion. At a given time ESYSROOT should be equal to EROOT, from
Unless you mean that the value of ROOT will be different between binpkg generation and installation.
Oh, sorry I didn't clarify. To me the dev container image is a production artifact that is shipped to users.
What does the gcc patch look like? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I wrote that line. 😀 It actually doesn't work for three reasons. As you say, 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 installation My There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i see, that's basically the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
sedargs+=( | ||||||
-e "s:(, *)?\" *${flag} *\"::g" | ||||||
-e "s:(, *)?' *${flag} *'::g" | ||||||
-e "s:,? ?${flag}\b::g" | ||||||
) | ||||||
done | ||||||
sed "${sedargs[@]}" "${FILES[@]}" || die | ||||||
} |
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
togrep
, but then\b
won't work…There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.