On Fri, 2025-05-23 at 20:02 -0400, Ionen Wolkens wrote:
> On Fri, May 23, 2025 at 06:40:38PM +0100, James Le Cuirot wrote:
> > The kernel we're building against may have had its tools (e.g. modpost)
> > built for the target arch or even some other arch than the we're
> > building with now. To work around this, rebuild those tools with make
> > modules_prepare when necessary.
> >
> > Doing this in pkg_setup is not ideal, but some ebuilds have their own
> > src_configure and src_compile functions. It will not trigger in the vast
> > majority of cases, and it is not done when installing binary packages.
> >
> > Signed-off-by: James Le Cuirot <ch...@gentoo.org>
> > ---
> > eclass/linux-mod-r1.eclass | 25 +++++++++++++++++++++++++
> > 1 file changed, 25 insertions(+)
> >
> > diff --git a/eclass/linux-mod-r1.eclass b/eclass/linux-mod-r1.eclass
> > index 16c280219ef89..a62f1b96bb5e9 100644
> > --- a/eclass/linux-mod-r1.eclass
> > +++ b/eclass/linux-mod-r1.eclass
> > @@ -352,6 +352,31 @@ linux-mod-r1_pkg_setup() {
> > _modules_set_makeargs
> >
> > _modules_sanity_gccplugins
> > +
> > + # Check whether modpost runs locally as it might have been built for a
> > + # different arch. It should do nothing successfully when called without
> > + # args. If it doesn't run, build it with make modules_prepare in a new
> > + # environment using just the necessary files, and repoint KV_OUT_DIR
> > there.
>
> I'd kind of prefer if this was split like the rest into e.g. an
> @INTERNAL _modules_prepare_cross or similar to match the rest of the
> eclass and call it here after _set_makeargs and before gccplugins
> because the latter builds using KV_OUT_DIR (not that it uses modpost,
> and also isn't a big deal if that sanity check fails for cross, no
> need to test it) and the former uses MAKEARGS.
>
> The above text or similar could be used for the function description
> (just split it at 72 chars, keep double spaces after periods, and
> put the function before _modules_prepare_kernel for alphabetical).
Good idea.
> And maybe start the function with:
>
> [[ ${CBUILD:-${CHOST}} == "${KERNEL_CHOST}" ]] && return
>
> It could avoid trying to set this up if modpost failed for another
> reason (sometime users have kind of broken builds), not that it'd
> be a big deal but may make errors more confusing.
I thought about doing something like that, but it can help in those other
cases like if modpost is just missing, although that obviously shouldn't
happen. One more useful case is if SYSROOT has the same tuple but a newer
glibc version.
> > + if ! "${KV_OUT_DIR}"/scripts/mod/modpost &>/dev/null; then
> > + # Try to run make modules_prepare in a new separate output
> > directory.
> > + # This cannot be done if there is not already a separate output
> > + # directory because the build system will complain that the
> > source
> > + # directory is not clean. In that case, copy the source
> > directory.
> > + if [[ ${KV_OUT_DIR} -ef ${KV_DIR} ]]; then
> > + cp -rLT --reflink=auto "${KV_OUT_DIR}"
> > "${WORKDIR}"/extmod-build || die
>
> fwiw the rest of the eclass uses --, be nice to have just to be
> consistent (aka ... --reflink=auto -- "${KV_OUT_DIR}" ...). Not that
> everything wouldn't explode either way if directories started with -
> (I use it only on principle).
>
> > + emake -C "${WORKDIR}"/extmod-build
> > "${MODULES_MAKEARGS[@]}" modules_prepare
> > + else
> > + mkdir "${WORKDIR}"/extmod-build || die
>
> mkdir -- too.
>
> > + # Don't use /proc/config.gz as it probably won't match
> > this kernel.
>
> Can drop this comment, feels superfluous. Using build .config is kind
> of obvious for modules, and /proc/config.gz may also not even exist
> if support is disabled.
>
> > + cp "${KV_OUT_DIR}"/{.config,Module.symvers}
> > "${WORKDIR}"/extmod-build || die
>
> cp --
>
> > + # KV_OUT_DIR may have been prepared with
> > install-extmod-build, which
> > + # doesn't include all the files needed to call make
> > modules_prepare,
> > + # so use the Makefile from the full kernel sources.
> > + emake -C "${WORKDIR}"/extmod-build
> > "${MODULES_MAKEARGS[@]}" modules_prepare \
> > + -f "${KERNEL_MAKEFILE}"
> > KBUILD_OUTPUT="${WORKDIR}"/extmod-build
> > + fi
> > + KV_OUT_DIR="${WORKDIR}"/extmod-build
>
> Can drop quotes here.
Okay to all that.
> That aside, while it is not shell readonly, linux-info calls that
> variable "A read-only variable" in its description, not a big deal
> but does make overriding it feel like an abuse of internal details.
In my head, I was thinking KV_OUT_DIR belonged to linux-mod-r1, so I take your
point now, but that can't really be helped.
> > + fi
> > }
> >
> > # @FUNCTION: linux-mod-r1_src_compile
> > --
> > 2.49.0
> >
>
> (haven't really looked at linux-info changes nor tested cross, but
> I trust that you know what you are doing here -- also linux-info is
> an inconsistent mess so no need to be picky there)
I'll tweak the logic slightly in my next version because I remembered that
KV_DIR can be dirty even if KV_OUT_DIR points elsewhere. I wish there was a
way to force it not to care about that.
Thanks for the review!
signature.asc
Description: This is a digitally signed message part
