diff mbox series

[RFC,v1] Dockerfile: Have "sandbox" build with i386 toolchain not x86_64 toolchain

Message ID 20241105165357.620796-1-trini@konsulko.com
State RFC
Delegated to: Tom Rini
Headers show
Series [RFC,v1] Dockerfile: Have "sandbox" build with i386 toolchain not x86_64 toolchain | expand

Commit Message

Tom Rini Nov. 5, 2024, 4:53 p.m. UTC
A long time ago, we had to use "i386" and "x86_64" toolchains for our
x86 platforms, depending on the target. We then moved to being able to
use the single "i386" toolchain to support all x86 platforms. At the
same time, we had been building sandbox with our "x86_64" toolchain in
CI. When x86 switched to being able to use a single toolchain we did not
update our CI buildman file to match. Do this now as we want to ensure
that sandbox is built with the same compiler version the rest of our
platforms are and not the host one.

Signed-off-by: Tom Rini <trini@konsulko.com>
---
Cc: Simon Glass <sjg@chromium.org>

I'm posting this as an RFC to start the discussion as when I mentioned
this to Simon on IRC, he was very much not in agreement.

My main argument is this, sandbox needs to use the same toolchain that
we compile (virtually) all other platforms with as one of the points is
to be able to catch warnings and compiler behavior changes as soon as
possible in our CI. And just like how developers can use
distribution-provided toolchains today, nothing changes in that regard,
we only make CI be more consistent now.
---
 tools/docker/Dockerfile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Simon Glass Nov. 5, 2024, 5:03 p.m. UTC | #1
Hi Tom,

On Tue, 5 Nov 2024 at 09:54, Tom Rini <trini@konsulko.com> wrote:
>
> A long time ago, we had to use "i386" and "x86_64" toolchains for our
> x86 platforms, depending on the target. We then moved to being able to
> use the single "i386" toolchain to support all x86 platforms. At the
> same time, we had been building sandbox with our "x86_64" toolchain in
> CI. When x86 switched to being able to use a single toolchain we did not
> update our CI buildman file to match. Do this now as we want to ensure
> that sandbox is built with the same compiler version the rest of our
> platforms are and not the host one.
>
> Signed-off-by: Tom Rini <trini@konsulko.com>
> ---
> Cc: Simon Glass <sjg@chromium.org>
>
> I'm posting this as an RFC to start the discussion as when I mentioned
> this to Simon on IRC, he was very much not in agreement.
>
> My main argument is this, sandbox needs to use the same toolchain that
> we compile (virtually) all other platforms with as one of the points is
> to be able to catch warnings and compiler behavior changes as soon as
> possible in our CI. And just like how developers can use
> distribution-provided toolchains today, nothing changes in that regard,
> we only make CI be more consistent now.

OK, that makes some sense to me, actually. It isn't the toolchain that
people will be using, but catching things earlier seems good.

The main concern I have is that we are not testing sandbox on the
toolchain that everyone will be using. But I haven't noticed failures
in CI which I cannot repeat locally, so we can cross that bridge when
we come to it.

Reviewed-by: Simon Glass <sjg@chromium.org>

> ---
>  tools/docker/Dockerfile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/docker/Dockerfile b/tools/docker/Dockerfile
> index 967ac89fbde8..e158bab6e570 100644
> --- a/tools/docker/Dockerfile
> +++ b/tools/docker/Dockerfile
> @@ -280,7 +280,7 @@ RUN /bin/echo -e "[toolchain]\nroot = /usr" > ~/.buildman
>  RUN /bin/echo -e "kernelorg = /opt/gcc-13.2.0-nolibc/*" >> ~/.buildman
>  RUN /bin/echo -e "\n[toolchain-prefix]\nxtensa = /opt/2020.07/xtensa-dc233c-elf/bin/xtensa-dc233c-elf-" >> ~/.buildman;
>  RUN /bin/echo -e "\n[toolchain-alias]\nsh = sh2" >> ~/.buildman
> -RUN /bin/echo -e "\nsandbox = x86_64" >> ~/.buildman
> +RUN /bin/echo -e "\nsandbox = i386" >> ~/.buildman
>  RUN /bin/echo -e "\nx86 = i386" >> ~/.buildman;
>
>  # Add mkbootimg tool
> --
> 2.43.0
>

Regards,
Simon
Simon Glass Nov. 9, 2024, 5:34 p.m. UTC | #2
Hi Tom,

On Tue, 5 Nov 2024 at 10:03, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Tom,
>
> On Tue, 5 Nov 2024 at 09:54, Tom Rini <trini@konsulko.com> wrote:
> >
> > A long time ago, we had to use "i386" and "x86_64" toolchains for our
> > x86 platforms, depending on the target. We then moved to being able to
> > use the single "i386" toolchain to support all x86 platforms. At the
> > same time, we had been building sandbox with our "x86_64" toolchain in
> > CI. When x86 switched to being able to use a single toolchain we did not
> > update our CI buildman file to match. Do this now as we want to ensure
> > that sandbox is built with the same compiler version the rest of our
> > platforms are and not the host one.
> >
> > Signed-off-by: Tom Rini <trini@konsulko.com>
> > ---
> > Cc: Simon Glass <sjg@chromium.org>
> >
> > I'm posting this as an RFC to start the discussion as when I mentioned
> > this to Simon on IRC, he was very much not in agreement.
> >
> > My main argument is this, sandbox needs to use the same toolchain that
> > we compile (virtually) all other platforms with as one of the points is
> > to be able to catch warnings and compiler behavior changes as soon as
> > possible in our CI. And just like how developers can use
> > distribution-provided toolchains today, nothing changes in that regard,
> > we only make CI be more consistent now.
>
> OK, that makes some sense to me, actually. It isn't the toolchain that
> people will be using, but catching things earlier seems good.
>
> The main concern I have is that we are not testing sandbox on the
> toolchain that everyone will be using. But I haven't noticed failures
> in CI which I cannot repeat locally, so we can cross that bridge when
> we come to it.
>
> Reviewed-by: Simon Glass <sjg@chromium.org>

This doesn't seem to work, for a few reasons:

1. Buildman doesn't support setting HOSTCC and CC unless using -O to
override the toolchain (so the setting in this patch has no effect)
2. Using the toolchains from kernel.org gives this error:

NO_LTO=1 make O=/tmp/b/sandbox defconfig all
HOSTCC=/home/sglass/.buildman-toolchains/gcc-13.2.0-nolibc/x86_64-linux/bin/x86_64-linux-x86_64-linux-gcc
CC=/home/sglass/.buildman-toolchains/gcc-13.2.0-nolibc/x86_64-linux/bin/x86_64-linux-x86_64-linux-gcc
 -j30 -s
/home/sglass/.buildman-toolchains/gcc-13.2.0-nolibc/x86_64-linux/bin/../lib/gcc/x86_64-linux/13.2.0/../../../../x86_64-linux/bin/ld:
cannot find crt1.o: No such file or directory
/home/sglass/.buildman-toolchains/gcc-13.2.0-nolibc/x86_64-linux/bin/../lib/gcc/x86_64-linux/13.2.0/../../../../x86_64-linux/bin/ld:
cannot find crti.o: No such file or directory
/home/sglass/.buildman-toolchains/gcc-13.2.0-nolibc/x86_64-linux/bin/../lib/gcc/x86_64-linux/13.2.0/../../../../x86_64-linux/bin/ld:
cannot find -lc: No such file or directory
/home/sglass/.buildman-toolchains/gcc-13.2.0-nolibc/x86_64-linux/bin/../lib/gcc/x86_64-linux/13.2.0/../../../../x86_64-linux/bin/ld:
cannot find crtn.o: No such file or directory
collect2: error: ld returned 1 exit status
make[3]: *** [scripts/Makefile.host:95: scripts/basic/fixdep] Error 1
make[2]: *** [/scratch/sglass/cosarm/src/third_party/u-boot/files/Makefile:503:
scripts_basic] Error 2
make[1]: *** [/scratch/sglass/cosarm/src/third_party/u-boot/files/Makefile:568:
__build_one_by_one] Error 2
make: *** [Makefile:177: sub-make] Error 2

Regards,
Simon
Tom Rini Nov. 13, 2024, 2:40 a.m. UTC | #3
On Sat, Nov 09, 2024 at 10:34:15AM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Tue, 5 Nov 2024 at 10:03, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Tom,
> >
> > On Tue, 5 Nov 2024 at 09:54, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > A long time ago, we had to use "i386" and "x86_64" toolchains for our
> > > x86 platforms, depending on the target. We then moved to being able to
> > > use the single "i386" toolchain to support all x86 platforms. At the
> > > same time, we had been building sandbox with our "x86_64" toolchain in
> > > CI. When x86 switched to being able to use a single toolchain we did not
> > > update our CI buildman file to match. Do this now as we want to ensure
> > > that sandbox is built with the same compiler version the rest of our
> > > platforms are and not the host one.
> > >
> > > Signed-off-by: Tom Rini <trini@konsulko.com>
> > > ---
> > > Cc: Simon Glass <sjg@chromium.org>
> > >
> > > I'm posting this as an RFC to start the discussion as when I mentioned
> > > this to Simon on IRC, he was very much not in agreement.
> > >
> > > My main argument is this, sandbox needs to use the same toolchain that
> > > we compile (virtually) all other platforms with as one of the points is
> > > to be able to catch warnings and compiler behavior changes as soon as
> > > possible in our CI. And just like how developers can use
> > > distribution-provided toolchains today, nothing changes in that regard,
> > > we only make CI be more consistent now.
> >
> > OK, that makes some sense to me, actually. It isn't the toolchain that
> > people will be using, but catching things earlier seems good.
> >
> > The main concern I have is that we are not testing sandbox on the
> > toolchain that everyone will be using. But I haven't noticed failures
> > in CI which I cannot repeat locally, so we can cross that bridge when
> > we come to it.
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> This doesn't seem to work, for a few reasons:
> 
> 1. Buildman doesn't support setting HOSTCC and CC unless using -O to
> override the toolchain (so the setting in this patch has no effect)

That shouldn't matter tho.

> 2. Using the toolchains from kernel.org gives this error:
> 
> NO_LTO=1 make O=/tmp/b/sandbox defconfig all
> HOSTCC=/home/sglass/.buildman-toolchains/gcc-13.2.0-nolibc/x86_64-linux/bin/x86_64-linux-x86_64-linux-gcc
> CC=/home/sglass/.buildman-toolchains/gcc-13.2.0-nolibc/x86_64-linux/bin/x86_64-linux-x86_64-linux-gcc
>  -j30 -s
> /home/sglass/.buildman-toolchains/gcc-13.2.0-nolibc/x86_64-linux/bin/../lib/gcc/x86_64-linux/13.2.0/../../../../x86_64-linux/bin/ld:
> cannot find crt1.o: No such file or directory
> /home/sglass/.buildman-toolchains/gcc-13.2.0-nolibc/x86_64-linux/bin/../lib/gcc/x86_64-linux/13.2.0/../../../../x86_64-linux/bin/ld:
> cannot find crti.o: No such file or directory
> /home/sglass/.buildman-toolchains/gcc-13.2.0-nolibc/x86_64-linux/bin/../lib/gcc/x86_64-linux/13.2.0/../../../../x86_64-linux/bin/ld:
> cannot find -lc: No such file or directory
> /home/sglass/.buildman-toolchains/gcc-13.2.0-nolibc/x86_64-linux/bin/../lib/gcc/x86_64-linux/13.2.0/../../../../x86_64-linux/bin/ld:
> cannot find crtn.o: No such file or directory
> collect2: error: ld returned 1 exit status
> make[3]: *** [scripts/Makefile.host:95: scripts/basic/fixdep] Error 1
> make[2]: *** [/scratch/sglass/cosarm/src/third_party/u-boot/files/Makefile:503:
> scripts_basic] Error 2
> make[1]: *** [/scratch/sglass/cosarm/src/third_party/u-boot/files/Makefile:568:
> __build_one_by_one] Error 2
> make: *** [Makefile:177: sub-make] Error 2

So yeah, here's the real problem. In the past the kernel.org toolchain
was (unintentionally I believe) good enough to make user-space programs
too. That's not the case now, and sandbox is really a userspace program
and not a bare-metal program.

So really the current line from the Dockerfile just needs to be removed.
diff mbox series

Patch

diff --git a/tools/docker/Dockerfile b/tools/docker/Dockerfile
index 967ac89fbde8..e158bab6e570 100644
--- a/tools/docker/Dockerfile
+++ b/tools/docker/Dockerfile
@@ -280,7 +280,7 @@  RUN /bin/echo -e "[toolchain]\nroot = /usr" > ~/.buildman
 RUN /bin/echo -e "kernelorg = /opt/gcc-13.2.0-nolibc/*" >> ~/.buildman
 RUN /bin/echo -e "\n[toolchain-prefix]\nxtensa = /opt/2020.07/xtensa-dc233c-elf/bin/xtensa-dc233c-elf-" >> ~/.buildman;
 RUN /bin/echo -e "\n[toolchain-alias]\nsh = sh2" >> ~/.buildman
-RUN /bin/echo -e "\nsandbox = x86_64" >> ~/.buildman
+RUN /bin/echo -e "\nsandbox = i386" >> ~/.buildman
 RUN /bin/echo -e "\nx86 = i386" >> ~/.buildman;
 
 # Add mkbootimg tool