[Buildroot] [PATCH v2 1/1] package: Add iotools package

[Buildroot] [PATCH v2 1/1] package: Add iotools package

[Andy Shevchenko]

The iotools package provides a set of simple command line tools which
allow access to hardware device registers. Supported register interfaces
include PCI, IO, memory mapped IO, SMBus, CPUID, and MSR. Also included
are some utilities which allow for simple arithmetic, logical, and other
operations.

If you ever have to debug hardware, you could probably use these tools.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

v2: removed explicit strip (Baruch), added to DEVELOPERS

 DEVELOPERS                   |  1 +
 package/Config.in            |  1 +
 package/iotools/Config.in    | 14 ++++++++++++++
 package/iotools/iotools.hash |  3 +++
 package/iotools/iotools.mk   | 20 ++++++++++++++++++++
 5 files changed, 39 insertions(+)
 create mode 100644 package/iotools/Config.in
 create mode 100644 package/iotools/iotools.hash
 create mode 100644 package/iotools/iotools.mk

diff --git a/DEVELOPERS b/DEVELOPERS
index c3b1d66b016b..158313be9335 100644
--- a/DEVELOPERS
+++ b/DEVELOPERS
@@ -230,6 +230,7 @@ F:	package/sysdig/
 
 N:	Andy Shevchenko <andy.shevchenko@gmail.com>
 F:	package/fb-test-app/
+F:	package/iotools/
 
 N:	Anisse Astier <anisse@astier.eu>
 F:	package/go/
diff --git a/package/Config.in b/package/Config.in
index bfbec1c21d04..08977a3a92db 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -533,6 +533,7 @@ endmenu
 	source "package/i7z/Config.in"
 	source "package/input-event-daemon/Config.in"
 	source "package/intel-microcode/Config.in"
+	source "package/iotools/Config.in"
 	source "package/ipmitool/Config.in"
 	source "package/ipmiutil/Config.in"
 	source "package/irda-utils/Config.in"
diff --git a/package/iotools/Config.in b/package/iotools/Config.in
new file mode 100644
index 000000000000..ffa1a3666738
--- /dev/null
+++ b/package/iotools/Config.in
@@ -0,0 +1,14 @@
+config BR2_PACKAGE_IOTOOLS
+	bool "iotools"
+	help
+	  Tools for poking all types of IO, supported are:
+	  - CMOS NVRAM (via /dev/nvram)
+	  - CPU ID (x86)
+	  - IO port (x86, PowerPC)
+	  - MMIO
+	  - MSR (x86)
+	  - PCI configuration space
+	  - SCOM (PowerPC)
+	  - SMBus
+
+	  https://github.com/andy-shev/iotools
diff --git a/package/iotools/iotools.hash b/package/iotools/iotools.hash
new file mode 100644
index 000000000000..11d7ffb73e39
--- /dev/null
+++ b/package/iotools/iotools.hash
@@ -0,0 +1,3 @@
+# Locally computed
+sha256  13b8b87c7d1f22b8d8f50c499af09f9414a0879d69c22393f1051ed4bf28f378  iotools-v1.7.tar.gz
+sha256  ab15fd526bd8dd18a9e77ebc139656bf4d33e97fc7238cd11bf60e2b9b8666c6  COPYING
diff --git a/package/iotools/iotools.mk b/package/iotools/iotools.mk
new file mode 100644
index 000000000000..03b87c89e9d0
--- /dev/null
+++ b/package/iotools/iotools.mk
@@ -0,0 +1,20 @@
+################################################################################
+#
+# iotools
+#
+################################################################################
+
+IOTOOLS_VERSION = v1.7
+IOTOOLS_SITE = $(call github,andy-shev,iotools,$(IOTOOLS_VERSION))
+IOTOOLS_LICENSE = GPL-2.0+
+IOTOOLS_LICENSE_FILES = COPYING
+
+define IOTOOLS_BUILD_CMDS
+	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D) CROSS_COMPILE=$(TARGET_CROSS) all
+endef
+
+define IOTOOLS_INSTALL_TARGET_CMDS
+	$(INSTALL) -m 0755 -t $(TARGET_DIR)/usr/bin -D $(@D)/iotools
+endef
+
+$(eval $(generic-package))
-- 
2.43.0.rc1.1336.g36b5255a03ac

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/1] package: Add iotools package

[Andy Yan]

Hi Shevchenko，
Thanks for your patch，I have a quick test with this tool, it can work on arm64 platform.
But i found it also  lacks some features that IO doesn't have, such as io cmd support a length
arguments, that make it can read It can read a batch of registers at once.
Would you consider adding these features in the future?

At 2024-09-28 02:34:59, "Andy Shevchenko" <andriy.shevchenko@linux.intel.com> wrote:
>The iotools package provides a set of simple command line tools which
>allow access to hardware device registers. Supported register interfaces
>include PCI, IO, memory mapped IO, SMBus, CPUID, and MSR. Also included
>are some utilities which allow for simple arithmetic, logical, and other
>operations.
>
>If you ever have to debug hardware, you could probably use these tools.
>
>Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>---
>
>v2: removed explicit strip (Baruch), added to DEVELOPERS
>
> DEVELOPERS                   |  1 +
> package/Config.in            |  1 +
> package/iotools/Config.in    | 14 ++++++++++++++
> package/iotools/iotools.hash |  3 +++
> package/iotools/iotools.mk   | 20 ++++++++++++++++++++
> 5 files changed, 39 insertions(+)
> create mode 100644 package/iotools/Config.in
> create mode 100644 package/iotools/iotools.hash
> create mode 100644 package/iotools/iotools.mk
>
>diff --git a/DEVELOPERS b/DEVELOPERS
>index c3b1d66b016b..158313be9335 100644
>--- a/DEVELOPERS
>+++ b/DEVELOPERS
>@@ -230,6 +230,7 @@ F:	package/sysdig/
> 
> N:	Andy Shevchenko <andy.shevchenko@gmail.com>
> F:	package/fb-test-app/
>+F:	package/iotools/
> 
> N:	Anisse Astier <anisse@astier.eu>
> F:	package/go/
>diff --git a/package/Config.in b/package/Config.in
>index bfbec1c21d04..08977a3a92db 100644
>--- a/package/Config.in
>+++ b/package/Config.in
>@@ -533,6 +533,7 @@ endmenu
> 	source "package/i7z/Config.in"
> 	source "package/input-event-daemon/Config.in"
> 	source "package/intel-microcode/Config.in"
>+	source "package/iotools/Config.in"
> 	source "package/ipmitool/Config.in"
> 	source "package/ipmiutil/Config.in"
> 	source "package/irda-utils/Config.in"
>diff --git a/package/iotools/Config.in b/package/iotools/Config.in
>new file mode 100644
>index 000000000000..ffa1a3666738
>--- /dev/null
>+++ b/package/iotools/Config.in
>@@ -0,0 +1,14 @@
>+config BR2_PACKAGE_IOTOOLS
>+	bool "iotools"
>+	help
>+	  Tools for poking all types of IO, supported are:
>+	  - CMOS NVRAM (via /dev/nvram)
>+	  - CPU ID (x86)
>+	  - IO port (x86, PowerPC)
>+	  - MMIO
>+	  - MSR (x86)
>+	  - PCI configuration space
>+	  - SCOM (PowerPC)
>+	  - SMBus
>+
>+	  https://github.com/andy-shev/iotools
>diff --git a/package/iotools/iotools.hash b/package/iotools/iotools.hash
>new file mode 100644
>index 000000000000..11d7ffb73e39
>--- /dev/null
>+++ b/package/iotools/iotools.hash
>@@ -0,0 +1,3 @@
>+# Locally computed
>+sha256  13b8b87c7d1f22b8d8f50c499af09f9414a0879d69c22393f1051ed4bf28f378  iotools-v1.7.tar.gz
>+sha256  ab15fd526bd8dd18a9e77ebc139656bf4d33e97fc7238cd11bf60e2b9b8666c6  COPYING
>diff --git a/package/iotools/iotools.mk b/package/iotools/iotools.mk
>new file mode 100644
>index 000000000000..03b87c89e9d0
>--- /dev/null
>+++ b/package/iotools/iotools.mk
>@@ -0,0 +1,20 @@
>+################################################################################
>+#
>+# iotools
>+#
>+################################################################################
>+
>+IOTOOLS_VERSION = v1.7
>+IOTOOLS_SITE = $(call github,andy-shev,iotools,$(IOTOOLS_VERSION))
>+IOTOOLS_LICENSE = GPL-2.0+
>+IOTOOLS_LICENSE_FILES = COPYING
>+
>+define IOTOOLS_BUILD_CMDS
>+	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D) CROSS_COMPILE=$(TARGET_CROSS) all
>+endef
>+
>+define IOTOOLS_INSTALL_TARGET_CMDS
>+	$(INSTALL) -m 0755 -t $(TARGET_DIR)/usr/bin -D $(@D)/iotools
>+endef
>+
>+$(eval $(generic-package))
>-- 
>2.43.0.rc1.1336.g36b5255a03ac
>
>_______________________________________________
>buildroot mailing list
>buildroot@buildroot.org
>https://lists.buildroot.org/mailman/listinfo/buildroot
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/1] package: Add iotools package

[Andy Shevchenko]

On Sat, Sep 28, 2024 at 05:10:32PM +0800, Andy Yan wrote:
> 
> Thanks for your patch，I have a quick test with this tool, it can work on arm64 platform.
> But i found it also  lacks some features that IO doesn't have, such as io cmd support a length
> arguments, that make it can read It can read a batch of registers at once.
> Would you consider adding these features in the future?

I'm not sure I got this. There are two commands mmio_dump (to read any memory
mapped IO range) and mmio_load (to write the range from a file).

> At 2024-09-28 02:34:59, "Andy Shevchenko" <andriy.shevchenko@linux.intel.com> wrote:
> >The iotools package provides a set of simple command line tools which
> >allow access to hardware device registers. Supported register interfaces
> >include PCI, IO, memory mapped IO, SMBus, CPUID, and MSR. Also included
> >are some utilities which allow for simple arithmetic, logical, and other
> >operations.
> >
> >If you ever have to debug hardware, you could probably use these tools.

-- 
With Best Regards,
Andy Shevchenko


_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/1] package: Add iotools package

[Andy Shevchenko]

On Fri, Sep 27, 2024 at 09:34:59PM +0300, Andy Shevchenko wrote:
> The iotools package provides a set of simple command line tools which
> allow access to hardware device registers. Supported register interfaces
> include PCI, IO, memory mapped IO, SMBus, CPUID, and MSR. Also included
> are some utilities which allow for simple arithmetic, logical, and other
> operations.
> 
> If you ever have to debug hardware, you could probably use these tools.

Can this be pulled eventually? Or is there any comment I have to address?

-- 
With Best Regards,
Andy Shevchenko


_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/1] package: Add iotools package

[Aaron Sierra]

---- On Mon, 14 Oct 2024 10:38:22 -0500 Andy Shevchenko  wrote ---
 >  
 > Can this be pulled eventually? Or is there any comment I have to address?
 >  
 > -- 
 > With Best Regards, 
 > Andy Shevchenko 
 >  

Hi Andy,
I contributed the {mem,mmio}_{load,dump} commands to your repo back in 2021 and had been considering submitting a package to Buildroot, since I find the utility so useful. It's the first package that I add to br2_external trees that I work with. Thanks for taking the lead.

There are a couple of things that I noticed in the patch that could easily be fixed.

1. Despite being tagged as v1.7, the Makefile VER_MINOR is still 6, so the resulting binary reports "version 1.6".
2. It would be nice if the package populated command symlinks in the target filesystem. That could be done in a number of way, like using an explicit list of commands maintained in the package Makefile (iotools.mk) or adding a host-build dependency and leveraging the --list-cmds output to get the list of embedded commands for linking.

-Aaron
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/1] package: Add iotools package

[Andy Shevchenko]

On Mon, Oct 14, 2024 at 12:00:48PM -0500, Aaron Sierra wrote:
> ---- On Mon, 14 Oct 2024 10:38:22 -0500 Andy Shevchenko  wrote ---
>  >  
>  > Can this be pulled eventually? Or is there any comment I have to address?
> 
> I contributed the {mem,mmio}_{load,dump} commands to your repo back in 2021
> and had been considering submitting a package to Buildroot, since I find the
> utility so useful. It's the first package that I add to br2_external trees
> that I work with. Thanks for taking the lead.
> 
> There are a couple of things that I noticed in the patch that could easily be
> fixed.
> 
> 1. Despite being tagged as v1.7, the Makefile VER_MINOR is still 6, so the
> resulting binary reports "version 1.6".

Thanks for catching this! I'll look at this at some point, have a lot of stuff
to do...

> 2. It would be nice if the package populated command symlinks in the target
> filesystem. That could be done in a number of way, like using an explicit
> list of commands maintained in the package Makefile (iotools.mk) or adding a
> host-build dependency and leveraging the --list-cmds output to get the list
> of embedded commands for linking.

No, I don't do this. I explained why in the reply to Vincent.

-- 
With Best Regards,
Andy Shevchenko


_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/1] package: Add iotools package

[Thomas Petazzoni via buildroot]

On Fri, 27 Sep 2024 21:34:59 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> The iotools package provides a set of simple command line tools which
> allow access to hardware device registers. Supported register interfaces
> include PCI, IO, memory mapped IO, SMBus, CPUID, and MSR. Also included
> are some utilities which allow for simple arithmetic, logical, and other
> operations.
> 
> If you ever have to debug hardware, you could probably use these tools.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---

Applied to master after doing some tweaks:

- Used STATIC=1/STATIC=0 to achieve the proper behavior with regard to
  BR2_STATIC_LIBS

- Use the canonical $(INSTALL) incantation for the installation step

One thing that isn't super nice is your handling of the DEBUG variable.
Ideally in Buildroot we would want neither DEBUG=0 nor DEBUG=1, but
"don't do anything and let Buildroot pass the CFLAGS it wants".

Thanks!

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

Re: [Buildroot] [PATCH v2 1/1] package: Add iotools package

[Andy Shevchenko]

On Sat, Oct 26, 2024 at 02:47:48PM +0200, Thomas Petazzoni via buildroot wrote:
> On Fri, 27 Sep 2024 21:34:59 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > The iotools package provides a set of simple command line tools which
> > allow access to hardware device registers. Supported register interfaces
> > include PCI, IO, memory mapped IO, SMBus, CPUID, and MSR. Also included
> > are some utilities which allow for simple arithmetic, logical, and other
> > operations.
> > 
> > If you ever have to debug hardware, you could probably use these tools.
> 
> Applied to master after doing some tweaks:

Thank you!

> - Used STATIC=1/STATIC=0 to achieve the proper behavior with regard to
>   BR2_STATIC_LIBS
> 
> - Use the canonical $(INSTALL) incantation for the installation step
> 
> One thing that isn't super nice is your handling of the DEBUG variable.
> Ideally in Buildroot we would want neither DEBUG=0 nor DEBUG=1, but
> "don't do anything and let Buildroot pass the CFLAGS it wants".

Any suggestion how it may be addressed?

-- 
With Best Regards,
Andy Shevchenko


_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/1] package: Add iotools package

[Thomas Petazzoni via buildroot]

Hello Andy,

On Mon, 28 Oct 2024 09:57:58 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> > One thing that isn't super nice is your handling of the DEBUG variable.
> > Ideally in Buildroot we would want neither DEBUG=0 nor DEBUG=1, but
> > "don't do anything and let Buildroot pass the CFLAGS it wants".  
> 
> Any suggestion how it may be addressed?

1. Don't override the CFLAGS passed by the user

2. Allow a value of DEBUG= that tells your Makefile to not do anything
   (i.e neither DEBUG=0 nor DEBUG=1)

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/1] package: Add iotools package

[Andy Shevchenko]

On Mon, Oct 28, 2024 at 09:26:00AM +0100, Thomas Petazzoni via buildroot wrote:
> On Mon, 28 Oct 2024 09:57:58 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> > > One thing that isn't super nice is your handling of the DEBUG variable.
> > > Ideally in Buildroot we would want neither DEBUG=0 nor DEBUG=1, but
> > > "don't do anything and let Buildroot pass the CFLAGS it wants".  
> > 
> > Any suggestion how it may be addressed?
> 
> 1. Don't override the CFLAGS passed by the user
> 
> 2. Allow a value of DEBUG= that tells your Makefile to not do anything
>    (i.e neither DEBUG=0 nor DEBUG=1)

Thanks!

Seems Aaron already does a lot to improve this by moving to CMake. I asked him
for a few changes in his PR and most likely we soon have the updated version in
Buildroot that satisfies the requirements.


-- 
With Best Regards,
Andy Shevchenko


_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/1] package: Add iotools package

[Thomas Petazzoni via buildroot]

On Thu, 31 Oct 2024 19:25:00 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Seems Aaron already does a lot to improve this by moving to CMake. I asked him
> for a few changes in his PR and most likely we soon have the updated version in
> Buildroot that satisfies the requirements.

Clearly using a standardized build system (CMake, meson, etc.) will
definitely help in having a more "regular" behavior.

Thanks!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/1] package: Add iotools package

[Vincent Fazio]

Aaron, Andy,

> -----Original Message-----
> From: buildroot <buildroot-bounces@buildroot.org> On Behalf Of Aaron
> Sierra
> Sent: Monday, October 14, 2024 12:01 PM
> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Andy Yan <andyshrk@gmail.com>; Andy Yan <andyshrk@163.com>;
> buildroot <buildroot@buildroot.org>
> Subject: [External] - Re: [Buildroot] [PATCH v2 1/1] package: Add iotools
> package
> 
> ---- On Mon, 14 Oct 2024 10:38:22 -0500 Andy Shevchenko  wrote ---  >  >
> Can this be pulled eventually? Or is there any comment I have to address?
>  >
>  > --
>  > With Best Regards,
>  > Andy Shevchenko
>  >
> 2. It would be nice if the package populated command symlinks in the target
> filesystem. That could be done in a number of way, like using an explicit list of
> commands maintained in the package Makefile (iotools.mk) or adding a host-
> build dependency and leveraging the --list-cmds output to get the list of
> embedded commands for linking.
> 
	
Caveat lector, I have not reviewed your patch or the sources in a while.

WRT to this comment, I have this note from when we ported it:

# we can't run make install because the symlink installation executes the binary
# which isn't guaranteed to work in cross-compile scenarios
define IOTOOLS_INSTALL_TARGET_CMDS
    $(INSTALL) -D -m 0755 $(@D)/iotools $(TARGET_DIR)/usr/sbin/

    for link in $( IOTOOLS_LINKS); do \
        ln -sf iotools $(TARGET_DIR)/usr/sbin/$$link; \
    done
endef


We then earlier in the mk file define the tools per platform, akin to:

# I/O utilities
IOTOOLS_LINKS = \
    cmos_read \
    cmos_write \
    mem_dump \
    mem_load \
    mem_read8 \
    mem_read16 \
    mem_read32 \
    mem_read64 \
    mem_write8 \
    mem_write16 \
    mem_write32 \
    mem_write64 \
    mmio_dump \
    mmio_load \
    mmio_read8 \
    mmio_read16 \
    mmio_read32 \
    mmio_read64 \
    mmio_write8 \
    mmio_write16 \
    mmio_write32 \
    mmio_write64 \
    io_read8 \
    io_read16 \
    io_read32 \
    io_write8 \
    io_write16 \
    io_write32 \
    pci_list \
    pci_read8 \
    pci_read16 \
    pci_read32 \
    pci_write8 \
    pci_write16 \
    pci_write32 \
    smbus_quick \
    smbus_read8 \
    smbus_read16 \
    smbus_readblock \
    smbus_receive_byte \
    smbus_send_byte \
    smbus_write8 \
    smbus_write16 \
    smbus_writeblock

# Bitwise utilities
IOTOOLS_LINKS += \
    and \
    brt \
    bts \
    not \
    or \
    shl \
    shr \
    xor

# Other utilities
IOTOOLS_LINKS += \
    busy_loop \
    cpu_list \
    runon

# Achitecture-specific utilities
ifeq ($(BR2_i386)$(BR2_x86_64),y)
IOTOOLS_LINKS += \
    cpuid \
    rdmsr \
    rdtsc \
    wrmsr
else ifeq ($(BR2_powerpc)$(BR2_powerpc64)$(BR2_powerpcle),y)
IOTOOLS_LINKS += \
    cputochipid \
    cputoex \
    getscom \
    putscom
endif

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/1] package: Add iotools package

[Andy Shevchenko]

On Mon, Oct 14, 2024 at 05:55:46PM +0000, Vincent Fazio wrote:
> Aaron, Andy,
> > From: buildroot <buildroot-bounces@buildroot.org> On Behalf Of Aaron
> > Sierra
> > Sent: Monday, October 14, 2024 12:01 PM
> > ---- On Mon, 14 Oct 2024 10:38:22 -0500 Andy Shevchenko  wrote ---
> > Can this be pulled eventually? Or is there any comment I have to address?

> > 2. It would be nice if the package populated command symlinks in the target
> > filesystem. That could be done in a number of way, like using an explicit list of
> > commands maintained in the package Makefile (iotools.mk) or adding a host-
> > build dependency and leveraging the --list-cmds output to get the list of
> > embedded commands for linking.
> 
> Caveat lector, I have not reviewed your patch or the sources in a while.
> 
> WRT to this comment, I have this note from when we ported it:
> 
> # we can't run make install because the symlink installation executes the binary
> # which isn't guaranteed to work in cross-compile scenarios
> define IOTOOLS_INSTALL_TARGET_CMDS
>     $(INSTALL) -D -m 0755 $(@D)/iotools $(TARGET_DIR)/usr/sbin/
> 
>     for link in $( IOTOOLS_LINKS); do \
>         ln -sf iotools $(TARGET_DIR)/usr/sbin/$$link; \
>     done
> endef
> 
> We then earlier in the mk file define the tools per platform, akin to:

I deliberately won't populate those links. The naming is so generic
that it's a real chance to get a collision with all pain of splitting it
in the configuration what links we create and what not and what to do
with those that may collide.

TL;DR: no, I don't think it's a good idea.

-- 
With Best Regards,
Andy Shevchenko


_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/1] package: Add iotools package

[Andy Shevchenko]

On Thu, Oct 17, 2024 at 10:45:23AM -0500, Aaron Sierra wrote:
>  > On Mon, Oct 14, 2024 at 05:55:46PM +0000, Vincent Fazio wrote: 
>  > > Aaron, Andy, 
>  > > > From: buildroot buildroot-bounces@buildroot.org> On Behalf Of Aaron 
>  > > > Sierra 
>  > > > Sent: Monday, October 14, 2024 12:01 PM 
>  > > > ---- On Mon, 14 Oct 2024 10:38:22 -0500 Andy Shevchenko  wrote --- 

...

>  > > > 2. It would be nice if the package populated command symlinks in the target 
>  > > > filesystem. That could be done in a number of way, like using an explicit list of 
>  > > > commands maintained in the package Makefile (iotools.mk) or adding a host- 
>  > > > build dependency and leveraging the --list-cmds output to get the list of 
>  > > > embedded commands for linking. 
>  > > 
>  > > Caveat lector, I have not reviewed your patch or the sources in a while. 
>  > > 
>  > > WRT to this comment, I have this note from when we ported it: 
>  > > 
>  > > # we can't run make install because the symlink installation executes the binary 
>  > > # which isn't guaranteed to work in cross-compile scenarios 
>  > > define IOTOOLS_INSTALL_TARGET_CMDS 
>  > >     $(INSTALL) -D -m 0755 $(@D)/iotools $(TARGET_DIR)/usr/sbin/ 
>  > > 
>  > >     for link in $( IOTOOLS_LINKS); do \ 
>  > >         ln -sf iotools $(TARGET_DIR)/usr/sbin/$link; \ 
>  > >     done 
>  > > endef 
>  > > 
>  > > We then earlier in the mk file define the tools per platform, akin to: 
>  >  
>  > I deliberately won't populate those links. The naming is so generic 
>  > that it's a real chance to get a collision with all pain of splitting it 
>  > in the configuration what links we create and what not and what to do 
>  > with those that may collide. 
>  >  
>  > TL;DR: no, I don't think it's a good idea. 
> 
> For the benefit of the uninitiated, iotools operates similarly to Busybox,
> expressing behavior either by:
>     1. taking the command name from argv[0] (i.e. symlinks named for internal commands)
>        * This is the mechanism encouraged by the project's make install (i.e.
>          iotools --make-links)
>     2. taking the command name from argv[1] (i.e. iotools [[command] [arguments]])

There is a little nuance between two. When the Busybox is the *main(
shell/coreutils/util-linux/etc provider it must populate the links, many of
them (naming wise) are standardized by POSIX. In full opposition to the iotools
with unpredictable consequences of the potential name clashing (esp. those that
are called or/xor/and/etc.).

> I proposed populating the symlinks in the target filesystem to ensure the
> subcommands are ready-to-use for people already familiar with the tool. It sounds
> like you're advocating for #2 ("iotools_fallback") to be the primary mode of
> operation for Buildroot users.

Yes, I highly suggest not to go this way.

> Can you point to documentation that Buildroot users can use to _know_ that the
> second mode of operation is available and encouraged? I do not see any in the
> help output or in the project README. Here is the current help output:
> 
> $ ./iotools --help
> usage: ./iotools COMMAND
>   COMMANDS:
>     --make-links
>     --clean-links
>     --list-cmds
>     -v --version

Is it full output? Because to me it shows two screens of the listed commands.
I do not see an issue here.

> For the sake of usability, I think it's important to either improve utility and/or
> project documentation or reconsider your stated position on symlink population.

I am against populating the links.

> Feel free to ask for help with either direction you choose.

-- 
With Best Regards,
Andy Shevchenko


_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/1] package: Add iotools package

[Andy Shevchenko]

On Fri, Oct 18, 2024 at 02:52:20PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 17, 2024 at 10:45:23AM -0500, Aaron Sierra wrote:
> >  > On Mon, Oct 14, 2024 at 05:55:46PM +0000, Vincent Fazio wrote: 
> >  > > Aaron, Andy, 
> >  > > > From: buildroot buildroot-bounces@buildroot.org> On Behalf Of Aaron 
> >  > > > Sierra 
> >  > > > Sent: Monday, October 14, 2024 12:01 PM 
> >  > > > ---- On Mon, 14 Oct 2024 10:38:22 -0500 Andy Shevchenko  wrote --- 

...

> >  > > > 2. It would be nice if the package populated command symlinks in the target 
> >  > > > filesystem. That could be done in a number of way, like using an explicit list of 
> >  > > > commands maintained in the package Makefile (iotools.mk) or adding a host- 
> >  > > > build dependency and leveraging the --list-cmds output to get the list of 
> >  > > > embedded commands for linking. 
> >  > > 
> >  > > Caveat lector, I have not reviewed your patch or the sources in a while. 
> >  > > 
> >  > > WRT to this comment, I have this note from when we ported it: 
> >  > > 
> >  > > # we can't run make install because the symlink installation executes the binary 
> >  > > # which isn't guaranteed to work in cross-compile scenarios 
> >  > > define IOTOOLS_INSTALL_TARGET_CMDS 
> >  > >     $(INSTALL) -D -m 0755 $(@D)/iotools $(TARGET_DIR)/usr/sbin/ 
> >  > > 
> >  > >     for link in $( IOTOOLS_LINKS); do \ 
> >  > >         ln -sf iotools $(TARGET_DIR)/usr/sbin/$link; \ 
> >  > >     done 
> >  > > endef 
> >  > > 
> >  > > We then earlier in the mk file define the tools per platform, akin to: 
> >  >  
> >  > I deliberately won't populate those links. The naming is so generic 
> >  > that it's a real chance to get a collision with all pain of splitting it 
> >  > in the configuration what links we create and what not and what to do 
> >  > with those that may collide. 
> >  >  
> >  > TL;DR: no, I don't think it's a good idea. 
> > 
> > For the benefit of the uninitiated, iotools operates similarly to Busybox,
> > expressing behavior either by:
> >     1. taking the command name from argv[0] (i.e. symlinks named for internal commands)
> >        * This is the mechanism encouraged by the project's make install (i.e.
> >          iotools --make-links)
> >     2. taking the command name from argv[1] (i.e. iotools [[command] [arguments]])
> 
> There is a little nuance between two. When the Busybox is the *main(
> shell/coreutils/util-linux/etc provider it must populate the links, many of
> them (naming wise) are standardized by POSIX. In full opposition to the iotools
> with unpredictable consequences of the potential name clashing (esp. those that
> are called or/xor/and/etc.).

Worth to add that when Busybox is supplied *in the addition to* the main shell
it must NOT populate the links (for the very same reasoning: POSIX standardization).

> > I proposed populating the symlinks in the target filesystem to ensure the
> > subcommands are ready-to-use for people already familiar with the tool. It sounds
> > like you're advocating for #2 ("iotools_fallback") to be the primary mode of
> > operation for Buildroot users.
> 
> Yes, I highly suggest not to go this way.
> 
> > Can you point to documentation that Buildroot users can use to _know_ that the
> > second mode of operation is available and encouraged? I do not see any in the
> > help output or in the project README. Here is the current help output:
> > 
> > $ ./iotools --help
> > usage: ./iotools COMMAND
> >   COMMANDS:
> >     --make-links
> >     --clean-links
> >     --list-cmds
> >     -v --version
> 
> Is it full output? Because to me it shows two screens of the listed commands.
> I do not see an issue here.
> 
> > For the sake of usability, I think it's important to either improve utility and/or
> > project documentation or reconsider your stated position on symlink population.
> 
> I am against populating the links.
> 
> > Feel free to ask for help with either direction you choose.

-- 
With Best Regards,
Andy Shevchenko


_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/1] package: Add iotools package

[Aaron Sierra]

Hi Andy,

 >  On Fri, 18 Oct 2024 06:52:20 -0500 Andy Shevchenko  wrote:
 > > On Thu, Oct 17, 2024 at 10:45:23AM -0500, Aaron Sierra wrote: 
 > > > On Mon, Oct 14, 2024 at 05:55:46PM +0000, Vincent Fazio wrote: 

[ snip iotools.mk symlink population reference from Vincent ]

 > > For the benefit of the uninitiated, iotools operates similarly to Busybox, 
 > > expressing behavior either by: 
 > >     1. taking the command name from argv[0] (i.e. symlinks named for internal commands) 
 > >        * This is the mechanism encouraged by the project's make install (i.e. 
 > >          iotools --make-links) 
 > >     2. taking the command name from argv[1] (i.e. iotools [[command] [arguments]]) 
 >  
 > There is a little nuance between two. When the Busybox is the *main( 
 > shell/coreutils/util-linux/etc provider it must populate the links, many of 
 > them (naming wise) are standardized by POSIX. In full opposition to the iotools 
 > with unpredictable consequences of the potential name clashing (esp. those that 
 > are called or/xor/and/etc.). 
 >  
 > > I proposed populating the symlinks in the target filesystem to ensure the 
 > > subcommands are ready-to-use for people already familiar with the tool. It sounds 
 > > like you're advocating for #2 ("iotools_fallback") to be the primary mode of 
 > > operation for Buildroot users. 
 >  
 > Yes, I highly suggest not to go this way. 
 >  
 > > Can you point to documentation that Buildroot users can use to _know_ that the 
 > > second mode of operation is available and encouraged? I do not see any in the 
 > > help output or in the project README. Here is the current help output: 
 > > 
 > > $ ./iotools --help 
 > > usage: ./iotools COMMAND 
 > >   COMMANDS: 
 > >     --make-links 
 > >     --clean-links 
 > >     --list-cmds 
 > >     -v --version 
 >  
 > Is it full output? Because to me it shows two screens of the listed commands. 
 > I do not see an issue here. 

That is the complete help output built from the v1.7 tag (751c76605f37).

 > > For the sake of usability, I think it's important to either improve utility and/or 
 > > project documentation or reconsider your stated position on symlink population. 
 >  
 > I am against populating the links. 

So, you are willing to improve project documentation then?

 > > Feel free to ask for help with either direction you choose. 

Be on the lookout for contributions to your repository that address my remaining concerns.

-Aaron
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/1] package: Add iotools package

[Aaron Sierra]

> On Mon, 21 Oct 2024 10:59:13 -0500 Aaron Sierra  wrote:
>
 > Hi Andy, 
 >  
 > Be on the lookout for contributions to your repository that address my remaining concerns. 
 >  

I've posted a pull request to your Github repo, https://github.com/andy-shev/iotools/pull/5,
that expands the runtime (and project) documentation, while adding support for symlink
installation in a cross-build environment.

-Aaron
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/1] package: Add iotools package

[Andy Shevchenko]

On Thu, Oct 24, 2024 at 03:30:26PM -0500, Aaron Sierra wrote:
> > On Mon, 21 Oct 2024 10:59:13 -0500 Aaron Sierra  wrote:
> >
>  > Be on the lookout for contributions to your repository that address my remaining concerns. 
> 
> I've posted a pull request to your Github repo, https://github.com/andy-shev/iotools/pull/5,
> that expands the runtime (and project) documentation, while adding support for symlink
> installation in a cross-build environment.

Thank you!
I just reviewed it, in general looks very good. Only few comments to address.

-- 
With Best Regards,
Andy Shevchenko


_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot