Re: [Buildroot] [PATCH v5 1/3] boot/xilinx-embeddedsw: rename toolchain vendor to buildroot

[Luca Ceresoli via buildroot <buildroot@buildroot.org>]

On Mon, 19 May 2025 16:02:33 +0000
"Frager, Neal" <neal.frager@amd.com> wrote:

> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> >  
> > > This patch renames the bare-metal toolchain vendor used by the
> > > xilinx-embeddedsw package from Xilinx to Buildroot to be consistent with all
> > > other toolchains built by Buildroot.
> > >
> > > To build the Microblaze applications available with the xilinx-embeddedsw
> > > package, the following config is now needed:
> > >
> > > BR2_TOOLCHAIN_BARE_METAL_BUILDROOT_ARCH="microblazeel-buildroot-elf"
> > >
> > > This change keeps backwards compatibility for users already using the
> > > following architecture tuple:
> > >
> > > BR2_TOOLCHAIN_BARE_METAL_BUILDROOT_ARCH="microblazeel-xilinx-elf"
> > >
> > > Either vendor name is now valid, but the documentation will describe using
> > > the Buildroot vendor name.
> > >
> > > Signed-off-by: Neal Frager <neal.frager@amd.com>
> > > ---
> > > V1->V2:
> > > - xilinx-embeddedsw is now backwards compatible with either vendor name
> > > V2->V3:
> > > - split patch into series
> > > V3->V4:
> > > - rebase patch
> > > V4->V5:
> > > - add deprecation warning for microblazeel-xilinx-elf tuple
> > > ---
> > >  boot/xilinx-embeddedsw/Config.in            |  3 ++-
> > >  boot/xilinx-embeddedsw/xilinx-embeddedsw.mk | 30 ++++++++++++++-------
> > >  2 files changed, 23 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/boot/xilinx-embeddedsw/Config.in b/boot/xilinx-embeddedsw/Config.in
> > > index a27253d594..31b12baaf2 100644
> > > --- a/boot/xilinx-embeddedsw/Config.in
> > > +++ b/boot/xilinx-embeddedsw/Config.in
> > > @@ -1,4 +1,5 @@
> > > -comment "xilinx-embeddedsw needs a bare metal toolchain for tuple microblazeel-xilinx-elf"
> > > +comment "xilinx-embeddedsw needs a bare metal toolchain for tuple microblazeel-buildroot-elf"
> > > +     depends on BR2_TOOLCHAIN_BARE_METAL_BUILDROOT_ARCH != "microblazeel-buildroot-elf"
> > >       depends on BR2_TOOLCHAIN_BARE_METAL_BUILDROOT_ARCH != "microblazeel-xilinx-elf"
> > >
> > >  menuconfig BR2_TARGET_XILINX_EMBEDDEDSW
> > > diff --git a/boot/xilinx-embeddedsw/xilinx-embeddedsw.mk b/boot/xilinx-embeddedsw/xilinx-embeddedsw.mk
> > > index 7d4fcf8b8f..6e37dc48bd 100644
> > > --- a/boot/xilinx-embeddedsw/xilinx-embeddedsw.mk
> > > +++ b/boot/xilinx-embeddedsw/xilinx-embeddedsw.mk
> > > @@ -12,6 +12,18 @@ XILINX_EMBEDDEDSW_INSTALL_TARGET = NO
> > >  XILINX_EMBEDDEDSW_INSTALL_IMAGES = YES
> > >  XILINX_EMBEDDEDSW_DEPENDENCIES = toolchain-bare-metal-buildroot
> > >
> > > +ifneq ("$(wildcard $(HOST_DIR)/bin/microblazeel-xilinx-elf-gcc)","")
> > > +$(warning BR2_TOOLCHAIN_BARE_METAL_BUILDROOT_ARCH="microblazeel-xilinx-elf" \
> > > +     will soon be deprecated!)
> > > +$(warning Please migrate to new bare-metal toolchain config below)  
> >  
> > > A warning is not very visible, so it won't be very effective. You
> > > should really use the same logic that Config.in.legacy uses: error out,
> > > and clearly state how to migrate, practically. E.g. (untested):  
> >  
> > > $(error microblazeel-xilinx-elf in BR2_TOOLCHAIN_BARE_METAL_BUILDROOT_ARCH is not supported anymore!)
> > > $(error Replace microblazeel-xilinx-elf with microblazeel-buildroot-elf in BR2_TOOLCHAIN_BARE_METAL_BUILDROOT_ARCH)  
> >
> > If I do this, then it means that users must switch immediately to the new
> > microblazeel-buildroot-elf tuple definition.  My thought about the warning was
> > that either tuple would be valid for awhile before deprecating the old one.  
> 
> > I understand your idea, and I'm sorry I didn't realize from the
> > beginning and changed my mind only after testing accurately the series.  
> 
> > However I think a warning will be unnoticed my many users, and others
> > might just be lazy and ignore it for as long as they can. So basically
> > they would never update their defconfigs until forced to do so.  
> 
> > It may look a rough attitude, but it's pragmatic, and it is what
> > happens with Config.in.legacy IIUC: an old symbol is not accepted
> > starting at a given Buildroot release, but an easy upgrade path is
> > provided for at least one year after that release. So users upgrading
> > regularly enough will:  
> 
> >  1) upgrade to newer Buildroot release
> >  2) have a build failure with instructions on how to update
> >  3) update their defconfig
> >  4) build successfully
> >  5) be happy  
> 
> > Note the "with instructions on how to update" at step 2. This is a
> > prerequisite to step 5. :-)  
> 
> Hi Luca,
> 
> I agree with your strategy of #2 leading to #5, so I will go ahead with
> deprecating the xilinx vendor name with this patch set.
> 
> However, to add the new buildroot vendor name, I have a chicken and
> egg issue.  If I deprecate the xilinx vendor name while adding the buildroot
> vendor name, it will cause all the zynqmp defconfigs to have build failures.
> 
> Would you be ok of my patch set doing the following?
> 
> Patch 1: Add buildroot vendor name with warning messages.
> Patch 2: Update toolchain-bare-metal help details.
> Patch 3: Migrate zynqmp defconfigs to new buildroot vendor.
> Patch 4: Change warning messages to error messages.

That would work. But if it makes your life simpler, I guess for this
specific case it would make sense to have a single commit doing
everything. I.e. squash all the 4 patches into a single patch.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot