Skip to content

tools: Update Unix.mk for CONFIG_ARCH_BOARD_COMMON #16241

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shtirlic
Copy link
Contributor

Summary

Preserve CONFIG_ARCH_BOARD_COMMON in make savedconfig tools command

Related: issue #12122

Impact

Fix the CONFIG_ARCH_BOARD_COMMON configurations

Testing

Tested with out of tree board config

@github-actions github-actions bot added Area: Tooling Area: Build system Size: XS The size of the change in this PR is very small labels Apr 20, 2025
@@ -753,6 +753,7 @@ savedefconfig: apps_preconfig
$(Q) grep "^CONFIG_ARCH_CHIP_" .config >> defconfig.tmp; true
$(Q) grep "CONFIG_ARCH_CHIP=" .config >> defconfig.tmp; true
$(Q) grep "CONFIG_ARCH_BOARD=" .config >> defconfig.tmp; true
$(Q) grep "CONFIG_ARCH_BOARD_COMMON=" .config >> defconfig.tmp; true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but, the change can't pass ci check

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you point me to the error, I don't understand what is falling in CI

Copy link
Contributor

@acassis acassis Apr 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shtirlic because this modification will include the common board to some boards that were adding it over other Kconfig, see the raspberrypi-pico-2 as example:

  [1/1] Normalize raspberrypi-pico-2/usbnsh
16a17
> CONFIG_ARCH_BOARD_COMMON=y
Saving the new configuration file
HEAD detached at pull/16241/merge

Inside arch/arm/Kconfig, there is this rule:

config ARCH_CHIP_RP23XX
        bool "Raspberry Pi RP23XX"
        select ARCH_CORTEXM33
        select ARCH_HAVE_RAMVECTORS
        select ARCH_HAVE_MULTICPU
        select ARCH_HAVE_I2CRESET
        select ARM_HAVE_WFE_SEV
        select ARCH_HAVE_CUSTOM_TESTSET
        select ARCH_HAVE_PWM_MULTICHAN
        select ARCH_BOARD_COMMON

So, ARCH_BOARD_COMMON were defined by default when the chip is RP23XX. I am NOT sure this is the right way, because this way user has no option to disable common board (if he wants to use his own board initialization drivers)

So, you will need to update all the board profiles using this command:

$ ./tools/refresh.sh --silent --defaults all

It will take some time, about 1 or 2h depending on your machine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@acassis got it, thank you, will force push the branch when the update is ready.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shtirlic maybe after you include the ARCH_BOARD_COMMON to the boards we could remove the ARCH_BOARD_COMMON from "config ARCH_CHIP_RP23XX". We need to align with original author about it. I think a chip definition shouldn't include a board policy like this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@acassis I still see the modified in CI job, but unable to determine what is the cause. whitespace or smth?

@github-actions github-actions bot added Board: arm Size: M The size of the change in this PR is medium and removed Size: XS The size of the change in this PR is very small labels Apr 21, 2025
@nuttxpr
Copy link

nuttxpr commented Apr 21, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the basic NuttX requirements, but it lacks crucial details. While it addresses the sections required, the content is too brief and lacks specifics. Here's why it's insufficient and what needs improvement:

  • Summary: While it mentions preserving CONFIG_ARCH_BOARD_COMMON and links a related issue, it doesn't explain why this change is necessary. What problem does issue Can't build custom board #12122 describe? How does preserving this config setting fix that problem? What part of the make savedconfig tool is changed?

  • Impact: Saying it "fixes configurations" isn't enough. Be explicit. Does this affect how users configure their boards? Does it change the build process itself? Are any specific architectures, boards, or drivers affected? Does this change require documentation updates? Are there any security implications to losing this setting (which is what the bug presumably caused)? Even if the answer to most of these is "NO", explicitly stating "NO" with brief justification is better than leaving them blank or vague.

  • Testing: "Tested with out of tree board config" is far too vague. Provide specific details:

    • Build Host: What operating system, CPU architecture, and compiler version did you use for building?
    • Target: What architecture and board configuration did you test on? Was this a simulator (qemu) or real hardware?
    • Logs: The "Testing logs before change" and "Testing logs after change" sections are empty. Include actual logs demonstrating the problem before the change and the corrected behavior after the change. Showing the difference the change makes is crucial. Even simple logs like the output of make savedconfig and the contents of the resulting .config file would be much better than nothing.

In short: While the PR touches on the required sections, it's missing the essential details that allow reviewers to understand the change, its impact, and its verification. More complete and specific information is needed for proper review and acceptance.

@acassis
Copy link
Contributor

acassis commented Apr 21, 2025

@lupyuen what is this error:

Configuration/Tool: qemu-armv8a/nsh_smp
2025-04-21 09:36:41
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Building NuttX...
make[1]: *** [Makefile:156: bin/lib_readlinkat.o] Error 127
make[1]: *** [Makefile:156: bin/lib_unlinkat.o] Error 127
make[1]: *** [Makefile:156: bin/lib_symlinkat.o] Error 127
make[1]: *** [Makefile:156: bin/lib_usleep.o] Error 127
make[1]: *** [Makefile:156: bin/lib_getpgrp.o] Error 127
make[1]: *** [Makefile:156: bin/lib_getpgid.o] Error 127
make[1]: *** [Makefile:156: bin/lib_lockf.o] Error 127
make[1]: *** [Makefile:156: bin/lib_flock.o] Error 127
make[1]: *** [Makefile:156: bin/lib_getpass.o] Error 127
make[1]: *** [Makefile:156: bin/lib_chdir.o] Error 127
make[1]: *** [Makefile:156: bin/lib_fchdir.o] Error 127
make[1]: *** [Makefile:156: bin/lib_setuid.o] Error 127
make[1]: *** [Makefile:156: bin/lib_setgid.o] Error 127
make[1]: *** [Makefile:156: bin/lib_getuid.o] Error 127
make[1]: *** [Makefile:156: bin/lib_getgid.o] Error 127
      0 [sig] make 41555 sig_send: error sending signal 11, pid 41555, pipe handle 0x144, nb 0, packsize 192, Win32 error 0
  15629 [main] make 41555 sig_send: error sending signal -72, pid 41555, pipe handle 0x144, nb 0, packsize 192, Win32 error 0
make[1]: *** [Makefile:156: bin/lib_seteuid.o] Error 127
1563507 [sig] make 41555 sig_send: error sending signal 11, pid 41555, pipe handle 0x144, nb 0, packsize 192, Win32 error 0
1579164 [main] make 41555 sig_send: error sending signal -72, pid 41555, pipe handle 0x144, nb 0, packsize 192, Win32 error 0
3126559 [sig] make 41555 sig_send: error sending signal 11, pid 41555, pipe handle 0x144, nb 0, packsize 192, Win32 error 0
3142184 [main] make 41555 sig_send: error sending signal -72, pid 41555, pipe handle 0x144, nb 0, packsize 192, Win32 error 0
make[1]: *** [Makefile:156: bin/lib_setegid.o] Error 127

@lupyuen
Copy link
Member

lupyuen commented Apr 21, 2025

@acassis It's a known but unsolved problem, restarting the CI Job will fix it:

@acassis
Copy link
Contributor

acassis commented Apr 21, 2025

@acassis It's a known but unsolved problem, restarting the CI Job will fix it:

* [[BUG] CI Job for msys2 fails intermittently with `make ... sig_send: error sending signal` #16010](https://github.com/apache/nuttx/issues/16010)

Thank you Lup, I already did it

@acassis
Copy link
Contributor

acassis commented Apr 23, 2025

@shtirlic please try to update only boards/arm/stm32f7/stm32f777zit6-meadow/configs/nsh/defconfig now:

$ ./tools/refresh.sh stm32f777zit6-meadow:nsh

If I merge your PR with this error all the new PRs will fail

Preserve CONFIG_ARCH_BOARD_COMMON in savedconfig, refresh boards
defconfigs

Related: issue apache#12122

Signed-off-by: Serg Podtynnyi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Build system Area: Tooling Board: arm Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants