nuttx

nuttx copied to clipboard

Reame
Issues

Merge the newlibc string into NuttX.

Open yangguangcai01 opened this issue 1 year ago • 4 comments

Summary

Merge the newlibc string into NuttX.

Impact

use optimizate c string.

Testing

ci

Aug 26 '24 12:08 yangguangcai01

All code come from https://github.com/bminor/newlib/tree/master/newlib/libc/string which doesn't declare copyright in source code. should we copy the full text from to https://github.com/bminor/newlib/blob/master/COPYING.NEWLIB to https://github.com/apache/nuttx/blob/master/LICENSE?

Aug 26 '24 16:08 xiaoxiang781216

@yangguangcai01 let add copyright to LICENSE file

Aug 28 '24 01:08 xiaoxiang781216

if this code has a different licence than NuttX then the build of these files must be guarded by CONFIG_ALLOW_XXX_COMPONENTS. Adding copyrights to the LICENSE file is not sufficient. But in case of this PR where files have no copyright headers, what option should be used ?

Aug 28 '24 13:08 raiden00pl

If there is no license in the file they inherit the license from LICENSE file. I would advise to handle with care files that go into the core NuttX since they may cause legal issues.

All this code should be scanned with a snippet scanner to ensure that we import the licenses for code that we think that we import and avoid pollution.

I would avoid this code and look for alternatives.

Aug 28 '24 13:08 jerpelea

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

Oct 16 '24 06:10 github-actions[bot]

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

Oct 16 '24 06:10 github-actions[bot]

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

Oct 16 '24 08:10 github-actions[bot]

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

Oct 16 '24 10:10 github-actions[bot]

but what is the final license of these files? is it BSD or Apache? Is there anyone who can clearly confirm this?

if it's BSD, it should depend on the ALLOW_BSD_COMPONENTS option, but since these are basic OS features, the entire OS should depend on the BSD by default

As the Summary said, "BSD License" We will update this PR, add 'ALLOW_BSD_COMPONENTS' depends.

Oct 16 '24 10:10 GUIDINGLI

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

Oct 16 '24 12:10 github-actions[bot]

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

Oct 16 '24 13:10 github-actions[bot]

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

Oct 16 '24 15:10 github-actions[bot]

@raiden00pl @jerpelea All license concern is addressed, please review again.

Oct 16 '24 15:10 xiaoxiang781216

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

Oct 16 '24 16:10 github-actions[bot]

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

Oct 17 '24 07:10 github-actions[bot]

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

Oct 17 '24 07:10 github-actions[bot]

what do you thing about creating a new configuration option like this:

config LIBC_STRING_OPTIMIZE
  depends on ALLOW_BSD_COMPONENTS
  default y
  --help--
    Use optimized string function implementation based on newlib.

this way, it'll be more visible to the user that feature like this exist. On the other hand, the user may want to disable this optimization (I assume it is more FLASH consuming).

Oct 17 '24 09:10 raiden00pl

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

Oct 17 '24 10:10 github-actions[bot]

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

Oct 17 '24 11:10 github-actions[bot]

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

Oct 17 '24 11:10 github-actions[bot]

what do you thing about creating a new configuration option like this:
config LIBC_STRING_OPTIMIZE
  depends on ALLOW_BSD_COMPONENTS
  default y
  --help--
    Use optimized string function implementation based on newlib.
this way, it'll be more visible to the user that feature like this exist. On the other hand, the user may want to disable this optimization (I assume it is more FLASH consuming).

ok, done

Oct 17 '24 11:10 yangguangcai01

Houston we gotta problem, I did a bisect from master head and looks like we broke configure with this one :-)

Kconfig file syntax seems invalid and CI did not catch it :-)

@yangguangcai01 can you please double check?

@xiaoxiang781216 @raiden00pl shall we revert or patch ?

% git bisect good
6e87f110833363d353b7c3e849d510d72749b67d is the first bad commit
commit 6e87f110833363d353b7c3e849d510d72749b67d
Author: yangguangcai <[email protected]>
Date:   Fri Jan 26 10:50:25 2024 +0800

    Merge the newlibc string into NuttX.

    Signed-off-by: yangguangcai <[email protected]>

 LICENSE                          |  31 +++++++++++++++++++++++++++++++
 libs/libc/string/Kconfig         |   7 +++++++
 libs/libc/string/lib_memccpy.c   |  96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 libs/libc/string/lib_memchr.c    | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 libs/libc/string/lib_memcmp.c    |  69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 libs/libc/string/lib_memcpy.c    |  70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 libs/libc/string/lib_memmove.c   |   8 +-------
 libs/libc/string/lib_memrchr.c   | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 libs/libc/string/lib_stpcpy.c    |  48 ++++++++++++++++++++++++++++++++++++++++++++++++
 libs/libc/string/lib_stpncpy.c   |  72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 libs/libc/string/lib_strcat.c    |  53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 libs/libc/string/lib_strchr.c    | 113 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 libs/libc/string/lib_strchrnul.c |   6 ++++++
 libs/libc/string/lib_strcmp.c    |  66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 libs/libc/string/lib_strcoll.c   |   2 +-
 libs/libc/string/lib_strcpy.c    |  53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 libs/libc/string/lib_strdup.c    |   2 +-
 libs/libc/string/lib_strlen.c    |  60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 libs/libc/string/lib_strncmp.c   |  85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 libs/libc/string/lib_strncpy.c   |  71 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 libs/libc/string/lib_strrchr.c   |  19 +++++++++++++++++++
 21 files changed, 1128 insertions(+), 9 deletions(-)


% git checkout master
% git checkout c113a224e8432cca8456b0f509b7716091594ee9

% git log -2
commit c113a224e8432cca8456b0f509b7716091594ee9 (HEAD)
Author: yangguangcai <[email protected]>
Date:   Mon Feb 26 22:59:52 2024 +0800

    newlibc:skip asan check.

    Signed-off-by: yangguangcai <[email protected]>

commit 6e87f110833363d353b7c3e849d510d72749b67d
Author: yangguangcai <[email protected]>
Date:   Fri Jan 26 10:50:25 2024 +0800

    Merge the newlibc string into NuttX.

    Signed-off-by: yangguangcai <[email protected]>


% ./tools/configure.sh -B nucleo-l432kc:nsh
  Copy files
  Select CONFIG_HOST_BSD=y
  Refreshing...
CP: arch/dummy/Kconfig to /XXX/nuttx.git/arch/dummy/dummy_kconfig
CP: boards/dummy/Kconfig to /XXX/nuttx.git/boards/dummy/dummy_kconfig
LN: platform/board to /XXX/nuttx-apps.git/platform/dummy
LN: include/arch to arch/arm/include
LN: include/arch/board to /XXX/nuttx.git/boards/arm/stm32l4/nucleo-l432kc/include
LN: drivers/platform to /XXX/nuttx.git/drivers/dummy
LN: include/arch/chip to /XXX/nuttx.git/arch/arm/include/stm32l4
LN: arch/arm/src/chip to /XXX/nuttx.git/arch/arm/src/stm32l4
LN: arch/arm/src/board to /XXX/nuttx.git/boards/arm/stm32l4/nucleo-l432kc/src
mkkconfig in /XXX/nuttx-apps.git/audioutils
mkkconfig in /XXX/nuttx-apps.git/benchmarks
mkkconfig in /XXX/nuttx-apps.git/boot
mkkconfig in /XXX/nuttx-apps.git/canutils
mkkconfig in /XXX/nuttx-apps.git/crypto
mkkconfig in /XXX/nuttx-apps.git/database
mkkconfig in /XXX/nuttx-apps.git/examples/mcuboot
mkkconfig in /XXX/nuttx-apps.git/examples/module
mkkconfig in /XXX/nuttx-apps.git/examples/sotest
mkkconfig in /XXX/nuttx-apps.git/examples
mkkconfig in /XXX/nuttx-apps.git/fsutils
mkkconfig in /XXX/nuttx-apps.git/games
mkkconfig in /XXX/nuttx-apps.git/graphics
mkkconfig in /XXX/nuttx-apps.git/industry
mkkconfig in /XXX/nuttx-apps.git/inertial
mkkconfig in /XXX/nuttx-apps.git/interpreters/luamodules
mkkconfig in /XXX/nuttx-apps.git/interpreters
mkkconfig in /XXX/nuttx-apps.git/logging
mkkconfig in /XXX/nuttx-apps.git/lte
mkkconfig in /XXX/nuttx-apps.git/math
mkkconfig in /XXX/nuttx-apps.git/mlearning
mkkconfig in /XXX/nuttx-apps.git/netutils
mkkconfig in /XXX/nuttx-apps.git/sdr
mkkconfig in /XXX/nuttx-apps.git/system
mkkconfig in /XXX/nuttx-apps.git/testing
mkkconfig in /XXX/nuttx-apps.git/videoutils
mkkconfig in /XXX/nuttx-apps.git/wireless/bluetooth
mkkconfig in /XXX/nuttx-apps.git/wireless/ieee802154
mkkconfig in /XXX/nuttx-apps.git/wireless
mkkconfig in /XXX/nuttx-apps.git
libs/libc/string/Kconfig:44: syntax error
libs/libc/string/Kconfig:43: unknown option "--help--"
libs/libc/string/Kconfig:44: unknown option "Use"
gmake: *** [tools/Unix.mk:696: olddefconfig] Error 1
ERROR: failed to refresh

Oct 17 '24 20:10 cederom

Same here, all the RISC-V Builds are broken: https://gist.github.com/lupyuen/974f625cdd57da30805277566dd404e5

$ tools/configure.sh rv-virt:knsh64
libs/libc/string/Kconfig:44: syntax error
libs/libc/string/Kconfig:43: unknown option "--help--"
libs/libc/string/Kconfig:44: unknown option "Use"
make: *** [olddefconfig] Error 1
ERROR: failed to refresh

Is it caused by the Kconfig version? Because the CI Build runs without errors: https://github.com/apache/nuttx/actions/runs/11387572001

Oct 18 '24 00:10 lupyuen

@yangguangcai01 We are missing hyphens in ---help---:

	--help--
		Use optimized string function implementation based on newlib.

Should be:

	---help---
		Use optimized string function implementation based on newlib.

Oct 18 '24 00:10 lupyuen

Thanks @lupyuen I have free moment here goes the fix https://github.com/apache/nuttx/pull/14388 :-)

Oct 18 '24 01:10 cederom

@cederom do you find the reason why nuttx ci doesn't catch this type of error?

Oct 18 '24 01:10 xiaoxiang781216

Wonder if CI is running a different version of Kconfig?

Oct 18 '24 01:10 lupyuen

@yangguangcai01 We are missing hyphens in ---help---:

	--help--
		Use optimized string function implementation based on newlib.

Should be:

	---help---
		Use optimized string function implementation based on newlib.

Thanks for your thoughts.

Oct 18 '24 01:10 yangguangcai01

I use kconfig-frontends-4.11.0.1 on FreeBSD :-)

Oct 18 '24 01:10 cederom

Wonder if CI is running a different version of Kconfig?

ci may use https://pypi.org/project/kconfiglib/, we can switch some back to use kconfig-frontend.

Oct 18 '24 01:10 xiaoxiang781216