Skip to content

Conversation

@nordicjm
Copy link
Contributor

These commits change how BOARD_* and SOC_* Kconfig variables are selected from being freely selectable in a build, e.g. if a build is configured for an STM32 platform then from menuconfig, it is possible to change to an atmel platform which results in a completely broken build - to a system whereby those variables can no longer be changed after a project is configured.

This is needed for future sysbuild integration work whereby filtering can be performed from a SOC_* and ARCH_* level.

Note: Conversion of nordic soc is currently absent

@nordicjm nordicjm requested a review from tejlmand September 12, 2022 11:30
@nordicjm nordicjm force-pushed the cmakesoc_comp branch 9 times, most recently from 09201cb to ce9534f Compare September 13, 2022 07:09
Copy link
Contributor

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

quick early glance.

Minor nit on the commit messages:

Make SOC Kconfig options unselectable

technically speaking the setting is still selectable (using select <config>), but it's not user selectable.

So a small proposal:

Make SOC Kconfig options internal

Comment on lines +18 to +19
Copy link
Contributor

Choose a reason for hiding this comment

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

i'm very much in favor of help texts, especially because they are rendered into the official documentation, but a help text which simply just adds the SoC name is not of much value, so for those I prefer to simply keep the config as-is, that is without a help text.

Copy link
Member

Choose a reason for hiding this comment

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

that does not apply everywhere, Nuvoton M48X Series MCU is much more descriptibe than SOC_SERIES_M48X. This commit is removing valuable information IMO.

Copy link
Contributor

@tejlmand tejlmand Sep 14, 2022

Choose a reason for hiding this comment

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

i'm only requesting not to add dummy help text where it is not existing.
(or improve the help text to be meaningful)

SoCs that has relevant help text should definitely keep that existing text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The text was already there, in the selection field, this just moves it from the selection field (as it is no longer selectable) to the help field

Kconfig.zephyr Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

this effectively changes Kconfig.defconfig from being mandatory to being optional.

The alternative approach is to keep the source and then boards not having any default to override must create an empty file, maybe containing # Intentionally left empty.

I have a slight preference for keeping the source and then an empty file for those boards not needing to override anything.

Reason being:

  • most downstream users of Zephyr will at some point need to create a custom board of there own, and having a source helps them to place board specific defconfigs the right place.
  • It prevents stupid user mistakes like writing kconfig.defconfig / Kconfig.defconfinstead of Kconfig.defconfig.
  • If board is later updated to override defaults then it's clearer where to do this if the file is already empty (but present)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer we use osource, we shouldn't create empty files if they aren't needed. I get the points you make, but there are enough boards that will have a Kconfig.defconfig that custom boards can see what the pattern is.

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough, we have different opinion on this.

Another risk is that someone later wants to add a new default to a board which doesn't have a Kconfig.defconfig already.
Contributor looks around in that board folder and decides to place it in Kconfig.$(BOARD) because lack of knowledge.
Such PR can easily approved and be merged, resulting in inconsistency.

Whereas a Kconfig.defconfig with content:

# Kconfig.defconfig file for board specific overload of default settings.

# No board specific defaults.

will help to ensure that such defaults goes to the right location.

Contributors will often look for an existing file to place small suggestions, rather than create new files.

@nordicjm nordicjm force-pushed the cmakesoc_comp branch 7 times, most recently from 7c90587 to b2697fb Compare September 13, 2022 14:23
@nordicjm nordicjm force-pushed the cmakesoc_comp branch 3 times, most recently from 521c774 to 116cb6d Compare September 14, 2022 10:12
@nordicjm
Copy link
Contributor Author

@stephanosio Is it possible to get an exclusion on needing copyright headers on these Kconfig files?

@stephanosio
Copy link
Member

@stephanosio Is it possible to get an exclusion on needing copyright headers on these Kconfig files?

To be discussed here: https://discord.com/channels/720317445772017664/989195595572973648/1019570877895036970

@stephanosio
Copy link
Member

Move BOARD_* and SOC_* configuration to unselectable Kconfigs

promptless would be a more correct wording.

@nordicjm nordicjm force-pushed the cmakesoc_comp branch 4 times, most recently from e34badb to 4684298 Compare September 14, 2022 14:06
Copy link
Contributor

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

if in anyway possible, we should only deprecated the old way of handling boards (and SoCs), as to still allow downstream users to keep using their existing, and old style boards.

A deprecation warning could be issued if we discover old style Kconfig.board and a missing Kconfig.${BOARD} file.

Feel free to ping directly for ideas on how this might be achieved, or provide reasons if this is not possible.

The BOARD Kconfig variable is now set by cmake, therefore boards no
longer need to set it manually themselves.

Signed-off-by: Jamie McCrae <[email protected]>
The BOARD Kconfig variable is now set by cmake, therefore boards no
longer need to set it manually themselves.

Signed-off-by: Jamie McCrae <[email protected]>
The BOARD Kconfig variable is now set by cmake, therefore boards no
longer need to set it manually themselves.

Signed-off-by: Jamie McCrae <[email protected]>
The BOARD Kconfig variable is now set by cmake, therefore boards no
longer need to set it manually themselves.

Signed-off-by: Jamie McCrae <[email protected]>
The BOARD Kconfig variable is now set by cmake, therefore boards no
longer need to set it manually themselves.

Signed-off-by: Jamie McCrae <[email protected]>
The BOARD Kconfig variable is now set by cmake, therefore boards no
longer need to set it manually themselves.

Signed-off-by: Jamie McCrae <[email protected]>
The BOARD Kconfig variable is now set by cmake, therefore boards no
longer need to set it manually themselves.

Signed-off-by: Jamie McCrae <[email protected]>
The BOARD Kconfig variable is now set by cmake, therefore boards no
longer need to set it manually themselves.

Signed-off-by: Jamie McCrae <[email protected]>
Adds a wildcard BOARD environmental value so that all board Kconfig
files are processed.

Signed-off-by: Jamie McCrae <[email protected]>
If checks on boards that do not have separate CPUs are redundant and
can be removed.

Signed-off-by: Jamie McCrae <[email protected]>
If checks on boards that do not have separate CPUs are redundant and
can be removed.

Signed-off-by: Jamie McCrae <[email protected]>
If checks on boards that do not have separate CPUs are redundant and
can be removed.

Signed-off-by: Jamie McCrae <[email protected]>
If checks on boards that do not have separate CPUs are redundant and
can be removed.

Signed-off-by: Jamie McCrae <[email protected]>
If checks on boards that do not have separate CPUs are redundant and
can be removed.

Signed-off-by: Jamie McCrae <[email protected]>
If checks on boards that do not have separate CPUs are redundant and
can be removed.

Signed-off-by: Jamie McCrae <[email protected]>
If checks on boards that do not have separate CPUs are redundant and
can be removed.

Signed-off-by: Jamie McCrae <[email protected]>
If checks on boards that do not have separate CPUs are redundant and
can be removed.

Signed-off-by: Jamie McCrae <[email protected]>
If checks on boards that do not have separate CPUs are redundant and
can be removed.

Signed-off-by: Jamie McCrae <[email protected]>
If checks on boards that do not have separate CPUs are redundant and
can be removed.

Signed-off-by: Jamie McCrae <[email protected]>
Updates the ifdef check to match renamed board Kconfigs.

Signed-off-by: Jamie McCrae <[email protected]>
Updates the ifdef check to match renamed board Kconfigs.

Signed-off-by: Jamie McCrae <[email protected]>
Updates the ifdef check to match renamed board Kconfigs.

Signed-off-by: Jamie McCrae <[email protected]>
Adds the new native_posix_64 supported board to the test.

Signed-off-by: Jamie McCrae <[email protected]>
@andyross
Copy link
Contributor

Some drive-by comments to clarify my position, though I really don't want to be dragged into a bikeshed and promise I'll resist continuing:

SOC selection is a feature which can be added to sysbuild, at present, sysbuild has no knowledge of the SOC it is being built for

So... platform selection might need to be at a higher level than raw kconfig, right? If it was something you could get via cmake via a short-circuit/trivial target before the build starts, that would fix all your problems, right?

The method zephyr is using is no different than the method that the linux kernel itself is using

But it is! Linux specifies architecture via an ARCH= variable passed to make[1], not via kconfig (though it mirrors it in kconfig). And while I don't know the history I strongly suspect it's for exactly this reason, to fix the chicken-and-egg dependency issues between platform layers that can't port like that. And this is exactly what I'm suggesting you investigate if you have the same problem.

Basically: if we're looking at a 2000-file PR, we need to be really sure we're solving the problem in the best way. I don't think this is what we want.

[1] And to be fair, Linux doesn't even try to solve the whole problem. The Linux equivalent to sysbuild is a bunch of distro/OS-supplied kernel/initrd packing/signing tools. And those know their platform a priori, they don't demand that Linux tell them. Which... is something I guess I'm not understanding about sysbuild, shouldn't it be starting with a solution to this problem rather than trying to squeeze it out of Zephyr?

@nordicjm
Copy link
Contributor Author

But it is! Linux specifies architecture via an ARCH= variable passed to make[1], not via kconfig (though it mirrors it in kconfig). And while I don't know the history I strongly suspect it's for exactly this reason, to fix the chicken-and-egg dependency issues between platform layers that can't port like that. And this is exactly what I'm suggesting you investigate if you have the same problem.

We do the same but from a board concept instead of arch, -DBOARD=x, hence why with the introduction of this PR you can no longer change the board, because doing so breaks the build, likewise in linux to change the arch would break the build, so needs to be done afresh.

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Nov 15, 2022
@github-actions github-actions bot closed this Nov 29, 2022
@nordicjm nordicjm deleted the cmakesoc_comp branch January 27, 2025 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants