[PATCH] Revert "config: tools only: add VIDEO to build bmp_logo"

[PATCH] Revert "config: tools only: add VIDEO to build bmp_logo"

[Fabio Estevam]

This reverts commit 1cfba53ca46cade2dbf4e067afc8c19e72909a4b.

Since commit 1cfba53ca46c ("config: tools only: add VIDEO to build
bmp_logo") the build of tools-only_defconfig fails:

| /bin/sh: line 1: tools/bmp_logo: No such file or directory

This has been noticed in OpenEmbedded and Debian [1].

Revert it for now.

[1] https://lists.denx.de/pipermail/u-boot/2023-January/504758.html

Signed-off-by: Fabio Estevam <festevam@denx.de>
---
 configs/tools-only_defconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/configs/tools-only_defconfig b/configs/tools-only_defconfig
index 2197063112..fb75235e3d 100644
--- a/configs/tools-only_defconfig
+++ b/configs/tools-only_defconfig
@@ -29,7 +29,6 @@ CONFIG_DM_RTC=y
 CONFIG_SOUND=y
 CONFIG_SYSRESET=y
 CONFIG_TIMER=y
-CONFIG_VIDEO=y
 # CONFIG_VIRTIO_MMIO is not set
 # CONFIG_VIRTIO_PCI is not set
 # CONFIG_VIRTIO_SANDBOX is not set
-- 
2.25.1

Re: [PATCH] Revert "config: tools only: add VIDEO to build bmp_logo"

[Peter Robinson]

On Thu, Jan 19, 2023 at 1:23 AM Fabio Estevam <festevam@denx.de> wrote:
>
> This reverts commit 1cfba53ca46cade2dbf4e067afc8c19e72909a4b.
>
> Since commit 1cfba53ca46c ("config: tools only: add VIDEO to build
> bmp_logo") the build of tools-only_defconfig fails:
>
> | /bin/sh: line 1: tools/bmp_logo: No such file or directory
>
> This has been noticed in OpenEmbedded and Debian [1].
>
> Revert it for now.

So how do you propose to fix the issue I have which this fixes?

> [1] https://lists.denx.de/pipermail/u-boot/2023-January/504758.html
>
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> ---
>  configs/tools-only_defconfig | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/configs/tools-only_defconfig b/configs/tools-only_defconfig
> index 2197063112..fb75235e3d 100644
> --- a/configs/tools-only_defconfig
> +++ b/configs/tools-only_defconfig
> @@ -29,7 +29,6 @@ CONFIG_DM_RTC=y
>  CONFIG_SOUND=y
>  CONFIG_SYSRESET=y
>  CONFIG_TIMER=y
> -CONFIG_VIDEO=y
>  # CONFIG_VIRTIO_MMIO is not set
>  # CONFIG_VIRTIO_PCI is not set
>  # CONFIG_VIRTIO_SANDBOX is not set
> --
> 2.25.1
>

Re: [PATCH] Revert "config: tools only: add VIDEO to build bmp_logo"

[Fabio Estevam]

Hi Peter,

On Thu, Jan 19, 2023 at 8:04 AM Peter Robinson <pbrobinson@gmail.com> wrote:

> So how do you propose to fix the issue I have which this fixes?

Ok,  we need some more information here:

- What is the exact problem you are trying to solve? How can we reproduce it?

- Is it a regression? What is the commit that caused the problem you
are trying to solve?

Re: [PATCH] Revert "config: tools only: add VIDEO to build bmp_logo"

[Peter Robinson]

On Thu, Jan 19, 2023 at 11:10 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Peter,
>
> On Thu, Jan 19, 2023 at 8:04 AM Peter Robinson <pbrobinson@gmail.com> wrote:
>
> > So how do you propose to fix the issue I have which this fixes?
>
> Ok,  we need some more information here:

Did you read the original thread?

> - What is the exact problem you are trying to solve? How can we reproduce it?

Building that tool with the tools only option.

> - Is it a regression? What is the commit that caused the problem you
> are trying to solve?

Re: [PATCH] Revert "config: tools only: add VIDEO to build bmp_logo"

[Fabio Estevam]

On Thu, Jan 19, 2023 at 8:18 AM Peter Robinson <pbrobinson@gmail.com> wrote:

> Did you read the original thread?

I only read your commit log and it is not clear if it is a regression
and which commit caused the problem.
It lacks a Fixes tag too.

> > - What is the exact problem you are trying to solve? How can we reproduce it?
>
> Building that tool with the tools only option.

When I try to build U-Boot 2023.01 u-boot-tools-native in OpenEmbedded
it works fine.

It fails when trying to build it for the target since commit
1cfba53ca46c ("config: tools only: add VIDEO to build
bmp_logo").

> > - Is it a regression? What is the commit that caused the problem you
> > are trying to solve?

What about this part?

Tom, Otavio? Any suggestions, please?

Re: [PATCH] Revert "config: tools only: add VIDEO to build bmp_logo"

[Peter Robinson]

On Thu, Jan 19, 2023 at 11:33 AM Fabio Estevam <festevam@gmail.com> wrote:
>
> On Thu, Jan 19, 2023 at 8:18 AM Peter Robinson <pbrobinson@gmail.com> wrote:
>
> > Did you read the original thread?
>
> I only read your commit log and it is not clear if it is a regression
> and which commit caused the problem.

I don't remember, this was back in November.

> It lacks a Fixes tag too.

Which are mostly pointless in U-Boot as it's not like the kernel where
there's stable release cycles.

> > > - What is the exact problem you are trying to solve? How can we reproduce it?
> >
> > Building that tool with the tools only option.
>
> When I try to build U-Boot 2023.01 u-boot-tools-native in OpenEmbedded
> it works fine.

But does it actually build the bmp_logo tool? It builds fine for me
too but that tool is no longer built.

> It fails when trying to build it for the target since commit
> 1cfba53ca46c ("config: tools only: add VIDEO to build
> bmp_logo").

Where when I build it for Fedora it builds for me and I get the
bmp_logo which we used to have pre 2023.01

> > > - Is it a regression? What is the commit that caused the problem you
> > > are trying to solve?
>
> What about this part?

I don't remember, see comment above.

Re: [PATCH] Revert "config: tools only: add VIDEO to build bmp_logo"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1736 bytes --]

On Thu, Jan 19, 2023 at 11:52:11AM +0000, Peter Robinson wrote:
> On Thu, Jan 19, 2023 at 11:33 AM Fabio Estevam <festevam@gmail.com> wrote:
> >
> > On Thu, Jan 19, 2023 at 8:18 AM Peter Robinson <pbrobinson@gmail.com> wrote:
> >
> > > Did you read the original thread?
> >
> > I only read your commit log and it is not clear if it is a regression
> > and which commit caused the problem.
> 
> I don't remember, this was back in November.
> 
> > It lacks a Fixes tag too.
> 
> Which are mostly pointless in U-Boot as it's not like the kernel where
> there's stable release cycles.

They're quite helpful, in order to track when changes are fixing a
problem that was introduced by some other commit.

> > > > - What is the exact problem you are trying to solve? How can we reproduce it?
> > >
> > > Building that tool with the tools only option.
> >
> > When I try to build U-Boot 2023.01 u-boot-tools-native in OpenEmbedded
> > it works fine.
> 
> But does it actually build the bmp_logo tool? It builds fine for me
> too but that tool is no longer built.
> 
> > It fails when trying to build it for the target since commit
> > 1cfba53ca46c ("config: tools only: add VIDEO to build
> > bmp_logo").
> 
> Where when I build it for Fedora it builds for me and I get the
> bmp_logo which we used to have pre 2023.01
> 
> > > > - Is it a regression? What is the commit that caused the problem you
> > > > are trying to solve?
> >
> > What about this part?
> 
> I don't remember, see comment above.

The unanswered question is, why does Fedora ship the bmp_logo tool? I'm
fairly certain it's one of the host tools that's not useful outside of
the build and so shouldn't be packaged.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Revert "config: tools only: add VIDEO to build bmp_logo"

[Peter Robinson]

On Thu, Jan 19, 2023 at 2:02 PM Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Jan 19, 2023 at 11:52:11AM +0000, Peter Robinson wrote:
> > On Thu, Jan 19, 2023 at 11:33 AM Fabio Estevam <festevam@gmail.com> wrote:
> > >
> > > On Thu, Jan 19, 2023 at 8:18 AM Peter Robinson <pbrobinson@gmail.com> wrote:
> > >
> > > > Did you read the original thread?
> > >
> > > I only read your commit log and it is not clear if it is a regression
> > > and which commit caused the problem.
> >
> > I don't remember, this was back in November.
> >
> > > It lacks a Fixes tag too.
> >
> > Which are mostly pointless in U-Boot as it's not like the kernel where
> > there's stable release cycles.
>
> They're quite helpful, in order to track when changes are fixing a
> problem that was introduced by some other commit.
>
> > > > > - What is the exact problem you are trying to solve? How can we reproduce it?
> > > >
> > > > Building that tool with the tools only option.
> > >
> > > When I try to build U-Boot 2023.01 u-boot-tools-native in OpenEmbedded
> > > it works fine.
> >
> > But does it actually build the bmp_logo tool? It builds fine for me
> > too but that tool is no longer built.
> >
> > > It fails when trying to build it for the target since commit
> > > 1cfba53ca46c ("config: tools only: add VIDEO to build
> > > bmp_logo").
> >
> > Where when I build it for Fedora it builds for me and I get the
> > bmp_logo which we used to have pre 2023.01
> >
> > > > > - Is it a regression? What is the commit that caused the problem you
> > > > > are trying to solve?
> > >
> > > What about this part?
> >
> > I don't remember, see comment above.
>
> The unanswered question is, why does Fedora ship the bmp_logo tool? I'm
> fairly certain it's one of the host tools that's not useful outside of
> the build and so shouldn't be packaged.

It pre dates my time as maintainer so I actually don't know other than
"we've basically shipped it for ever and it's impossible to tell if it
has users", we could probably drop it and wait to see if I get bug
reports.

Re: [PATCH] Revert "config: tools only: add VIDEO to build bmp_logo"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2373 bytes --]

On Fri, Jan 20, 2023 at 08:30:27AM +0000, Peter Robinson wrote:
> On Thu, Jan 19, 2023 at 2:02 PM Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Jan 19, 2023 at 11:52:11AM +0000, Peter Robinson wrote:
> > > On Thu, Jan 19, 2023 at 11:33 AM Fabio Estevam <festevam@gmail.com> wrote:
> > > >
> > > > On Thu, Jan 19, 2023 at 8:18 AM Peter Robinson <pbrobinson@gmail.com> wrote:
> > > >
> > > > > Did you read the original thread?
> > > >
> > > > I only read your commit log and it is not clear if it is a regression
> > > > and which commit caused the problem.
> > >
> > > I don't remember, this was back in November.
> > >
> > > > It lacks a Fixes tag too.
> > >
> > > Which are mostly pointless in U-Boot as it's not like the kernel where
> > > there's stable release cycles.
> >
> > They're quite helpful, in order to track when changes are fixing a
> > problem that was introduced by some other commit.
> >
> > > > > > - What is the exact problem you are trying to solve? How can we reproduce it?
> > > > >
> > > > > Building that tool with the tools only option.
> > > >
> > > > When I try to build U-Boot 2023.01 u-boot-tools-native in OpenEmbedded
> > > > it works fine.
> > >
> > > But does it actually build the bmp_logo tool? It builds fine for me
> > > too but that tool is no longer built.
> > >
> > > > It fails when trying to build it for the target since commit
> > > > 1cfba53ca46c ("config: tools only: add VIDEO to build
> > > > bmp_logo").
> > >
> > > Where when I build it for Fedora it builds for me and I get the
> > > bmp_logo which we used to have pre 2023.01
> > >
> > > > > > - Is it a regression? What is the commit that caused the problem you
> > > > > > are trying to solve?
> > > >
> > > > What about this part?
> > >
> > > I don't remember, see comment above.
> >
> > The unanswered question is, why does Fedora ship the bmp_logo tool? I'm
> > fairly certain it's one of the host tools that's not useful outside of
> > the build and so shouldn't be packaged.
> 
> It pre dates my time as maintainer so I actually don't know other than
> "we've basically shipped it for ever and it's impossible to tell if it
> has users", we could probably drop it and wait to see if I get bug
> reports.

OK, thanks, we'll revert this and see what the fall out, if any, ends up
being.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Revert "config: tools only: add VIDEO to build bmp_logo"

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

On Wed, Jan 18, 2023 at 10:23:21PM -0300, Fabio Estevam wrote:

> This reverts commit 1cfba53ca46cade2dbf4e067afc8c19e72909a4b.
> 
> Since commit 1cfba53ca46c ("config: tools only: add VIDEO to build
> bmp_logo") the build of tools-only_defconfig fails:
> 
> | /bin/sh: line 1: tools/bmp_logo: No such file or directory
> 
> This has been noticed in OpenEmbedded and Debian [1].
> 
> Revert it for now.
> 
> [1] https://lists.denx.de/pipermail/u-boot/2023-January/504758.html
> 
> Signed-off-by: Fabio Estevam <festevam@denx.de>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] Revert "config: tools only: add VIDEO to build bmp_logo"

[Rasmus Villemoes]

On 20/01/2023 16.23, Tom Rini wrote:
> On Fri, Jan 20, 2023 at 08:30:27AM +0000, Peter Robinson wrote:
>> On Thu, Jan 19, 2023 at 2:02 PM Tom Rini <trini@konsulko.com> wrote:
>>>
>>> On Thu, Jan 19, 2023 at 11:52:11AM +0000, Peter Robinson wrote:
>>>> On Thu, Jan 19, 2023 at 11:33 AM Fabio Estevam <festevam@gmail.com> wrote:
>>>>>
>>>>> On Thu, Jan 19, 2023 at 8:18 AM Peter Robinson <pbrobinson@gmail.com> wrote:
>>>>>
>>>>>> Did you read the original thread?
>>>>>
>>>>> I only read your commit log and it is not clear if it is a regression
>>>>> and which commit caused the problem.
>>>>
>>>> I don't remember, this was back in November.
>>>>
>>>>> It lacks a Fixes tag too.
>>>>
>>>> Which are mostly pointless in U-Boot as it's not like the kernel where
>>>> there's stable release cycles.
>>>
>>> They're quite helpful, in order to track when changes are fixing a
>>> problem that was introduced by some other commit.
>>>
>>>>>>> - What is the exact problem you are trying to solve? How can we reproduce it?
>>>>>>
>>>>>> Building that tool with the tools only option.
>>>>>
>>>>> When I try to build U-Boot 2023.01 u-boot-tools-native in OpenEmbedded
>>>>> it works fine.
>>>>
>>>> But does it actually build the bmp_logo tool? It builds fine for me
>>>> too but that tool is no longer built.
>>>>
>>>>> It fails when trying to build it for the target since commit
>>>>> 1cfba53ca46c ("config: tools only: add VIDEO to build
>>>>> bmp_logo").
>>>>
>>>> Where when I build it for Fedora it builds for me and I get the
>>>> bmp_logo which we used to have pre 2023.01
>>>>
>>>>>>> - Is it a regression? What is the commit that caused the problem you
>>>>>>> are trying to solve?
>>>>>
>>>>> What about this part?
>>>>
>>>> I don't remember, see comment above.
>>>
>>> The unanswered question is, why does Fedora ship the bmp_logo tool? I'm
>>> fairly certain it's one of the host tools that's not useful outside of
>>> the build and so shouldn't be packaged.
>>
>> It pre dates my time as maintainer so I actually don't know other than
>> "we've basically shipped it for ever and it's impossible to tell if it
>> has users", we could probably drop it and wait to see if I get bug
>> reports.
> 
> OK, thanks, we'll revert this and see what the fall out, if any, ends up
> being.
> 

So we also hit this problem, not with the target version of u-boot-tools
(which we don't build), but the nativesdk one, with the same symptoms.
We certainly don't use or need the bmp_logo tool, so we're happy with
the revert and will just carry that on top of v2023.01 for now, but
since I spent some time on this I thought I might as well float the fix
I was about to send, before I noticed the revert:

diff --git a/tools/Makefile b/tools/Makefile
index edfa40903d..5c8175cf2a 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -281,8 +281,10 @@ hostprogs-y += printinitialenv
 # Generated LCD/video logo
 LOGO_H = $(objtree)/include/bmp_logo.h
 LOGO_DATA_H = $(objtree)/include/bmp_logo_data.h
+ifeq ($(CROSS_BUILD_TOOLS),)
 LOGO-$(CONFIG_VIDEO_LOGO) += $(LOGO_H)
 LOGO-$(CONFIG_VIDEO_LOGO) += $(LOGO_DATA_H)
+endif

 # Generic logo
 ifeq ($(LOGO_BMP),)

This relies on the Yocto recipe doing (essentially)

      make tools-only_defconfig
      make cross_tools NO_SDL=1

so the top Makefile sets CROSS_BUILD_TOOLS, and thus we avoid adding the
targets that need to invoke the bmp_logo tool (which obviously doesn't
work when built for target, nor when built for nativesdk where the ELF
file ends up with "interpreter
/usr/local/oe-sdk-hardcoded-buildpath/sysroots/x86_64-oesdk-linux/lib/ld-linux-x86-64.so.2"),
while still allowing the bmp_logo tool itself to be built.

Rasmus

Re: [PATCH] Revert "config: tools only: add VIDEO to build bmp_logo"

[Rasmus Villemoes]

On 25/01/2023 15.28, Rasmus Villemoes wrote:
> On 20/01/2023 16.23, Tom Rini wrote:
>> On Fri, Jan 20, 2023 at 08:30:27AM +0000, Peter Robinson wrote:
>>> On Thu, Jan 19, 2023 at 2:02 PM Tom Rini <trini@konsulko.com> wrote:
>>>>
>>>> On Thu, Jan 19, 2023 at 11:52:11AM +0000, Peter Robinson wrote:
>>>>> On Thu, Jan 19, 2023 at 11:33 AM Fabio Estevam <festevam@gmail.com> wrote:
>>>>>>
>>>>>> On Thu, Jan 19, 2023 at 8:18 AM Peter Robinson <pbrobinson@gmail.com> wrote:
>>>>>>
>>>>>>> Did you read the original thread?
>>>>>>
>>>>>> I only read your commit log and it is not clear if it is a regression
>>>>>> and which commit caused the problem.
>>>>>
>>>>> I don't remember, this was back in November.
>>>>>
>>>>>> It lacks a Fixes tag too.
>>>>>
>>>>> Which are mostly pointless in U-Boot as it's not like the kernel where
>>>>> there's stable release cycles.
>>>>
>>>> They're quite helpful, in order to track when changes are fixing a
>>>> problem that was introduced by some other commit.

So I also got curious about that part, because when I build
tools-only_defconfig with older U-Boot releases I don't get a bmp_logo
tool built.

I think I figured that out (but only after it's been revealed that this
was about Fedora's packaging of u-boot-tools). Fedora seems to do

%make_build HOSTCC="gcc $RPM_OPT_FLAGS" CROSS_COMPILE=""
tools-only_defconfig O=builds/
	
%make_build HOSTCC="gcc $RPM_OPT_FLAGS" CROSS_COMPILE="" tools-all O=builds/

so unlike the Yocto case, the build target is tools-all and not
cross_tools. Hence the top Makefile sets HOST_TOOLS_ALL=y, which in
tools/Makefile then hits

ifneq ($(HOST_TOOLS_ALL),)
CONFIG_ARCH_KIRKWOOD = y
...
endif

and that block used to, until 1dc6517649f2, contain

CONFIG_LCD_LOGO = y

with the corresponding

hostprogs-$(CONFIG_LCD_LOGO) += bmp_logo

further down.

IOW, it wasn't really tools-only_defconfig per se that used to cause
bmp_logo to get built, it was the tools-all target.

Rasmus

Re: [PATCH] Revert "config: tools only: add VIDEO to build bmp_logo"

[Fabio Estevam]

Hi Rasmus,

On Wed, Jan 25, 2023 at 11:59 AM Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:

> So I also got curious about that part, because when I build
> tools-only_defconfig with older U-Boot releases I don't get a bmp_logo
> tool built.
>
> I think I figured that out (but only after it's been revealed that this
> was about Fedora's packaging of u-boot-tools). Fedora seems to do
>
> %make_build HOSTCC="gcc $RPM_OPT_FLAGS" CROSS_COMPILE=""
> tools-only_defconfig O=builds/
>
> %make_build HOSTCC="gcc $RPM_OPT_FLAGS" CROSS_COMPILE="" tools-all O=builds/
>
> so unlike the Yocto case, the build target is tools-all and not
> cross_tools. Hence the top Makefile sets HOST_TOOLS_ALL=y, which in
> tools/Makefile then hits
>
> ifneq ($(HOST_TOOLS_ALL),)
> CONFIG_ARCH_KIRKWOOD = y
> ...
> endif
>
> and that block used to, until 1dc6517649f2, contain
>
> CONFIG_LCD_LOGO = y
>
> with the corresponding
>
> hostprogs-$(CONFIG_LCD_LOGO) += bmp_logo
>
> further down.
>
> IOW, it wasn't really tools-only_defconfig per se that used to cause
> bmp_logo to get built, it was the tools-all target.

Thanks for the in-depth investigation, appreciated it.