Skip to content
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

cmake: add support for user-defined board aliases #24094

Merged
merged 2 commits into from
Apr 9, 2020

Conversation

pabigot
Copy link
Collaborator

@pabigot pabigot commented Apr 4, 2020

Inspired by the solution supporting deprecated board names for the Nordic rename, and motivated by a long standing annoyance at having to type out things like nrf52840_pca10056 when I'm identifying the board I want to build for.

Zephyr board names must be globally unique which requires that they
encode all necessary information to identify a specific target.
Typing in these names can be inconvenient to developers working on
multiple targets within a single workspace.

Extend the cmake infrastructure to read an optional board aliases file
that will map custom aliases to the corresponding canonical Zephyr
board name.

Signed-off-by: Peter A. Bigot pab@pabigot.com

include(${ZEPHYR_BASE}/board_aliases.cmake OPTIONAL)
if(${BOARD}_BOARD_ALIAS)
set(BOARD_ALIAS ${BOARD} CACHE STRING "Board alias, provided by user")
set(BOARD ${${BOARD}_BOARD_ALIAS})
Copy link
Member

Choose a reason for hiding this comment

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

Will this work for overlays?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the overlay is named using the canonical name it should. It won't find the overlay if you name it using the alias. IMO it shouldn't; the alias is just for personal convenience.

@pabigot pabigot force-pushed the pabigot/20200404a branch from 795a1a4 to 578dd9b Compare April 4, 2020 17:51
@pabigot
Copy link
Collaborator Author

pabigot commented Apr 4, 2020

I've updated this so it now looks for an environment variable that specifies a file to be included for this purpose.

@nashif is this better? Since this file isn't written by the Zephyr infrastructure, and for now there's only one, I don't see that we need to identify a directory to hold it.

Copy link
Member

@carlescufi carlescufi left a comment

Choose a reason for hiding this comment

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

Very nice, thank you.

@carlescufi
Copy link
Member

@tejlmand are you OK with this change?

Copy link
Collaborator

@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.

Changes themselves looks good.

Just a small thought.
In principle, this makes it possible for a user to accidential hide an exisitng board name.
Probably not somthing that will happen for an nrf-named board, but could happen for example like this:

set(qemu_x86_BOARD_ALIAS qemu_x86_nommu)

in which case the user cannot select the real qemu_x86.

For sure, this is a user error, but I suggest we check the alias, to only allow using an unused board name.

Deprecated board list is different, as it is already ensured in that list, that name is not colliding with real board name as part of PR review.

@pabigot
Copy link
Collaborator Author

pabigot commented Apr 6, 2020

For sure, this is a user error, but I suggest we check the alias, to only allow using an unused board name.

I don't think this is a reasonable concern, and don't want to spend a lot of effort trying to cope with it. The user has complete control over her aliases. If she chooses to use a name to replace an official board, thus blocking access to it, why should that matter and why should we stop her?

But if you can you propose a simple way to do such a check, and others think it's worth it, I don't strongly object to adding it.

@tejlmand
Copy link
Collaborator

tejlmand commented Apr 7, 2020

For sure, this is a user error, but I suggest we check the alias, to only allow using an unused board name.

I don't think this is a reasonable concern, and don't want to spend a lot of effort trying to cope with it. The user has complete control over her aliases. If she chooses to use a name to replace an official board, thus blocking access to it, why should that matter and why should we stop her?

But if you can you propose a simple way to do such a check, and others think it's worth it, I don't strongly object to adding it.

Done:
pabigot#1

  if (BOARD_ALIAS)
    find_path(BOARD_HIDDEN_DIR
      NAMES ${BOARD_ALIAS}_defconfig
      PATHS ${root}/boards/*/*
      NO_DEFAULT_PATH
      )
    if(BOARD_HIDDEN_DIR)
      message(WARNING "Board alias ${BOARD_ALIAS} is hiding the real board of same name")
    endif()
  endif()

and will print:

Aliased BOARD=nrf51dk_nrf51422 changed to qemu_x86.
CMake Warning at /projects/github/zephyr-upstream/zephyr/cmake/app/boilerplate.cmake:302 (message):
  Board alias nrf51dk_nrf51422 is hiding the real board of same name

Feel free to take in the suggestion.

Copy link
Collaborator

@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.

Comments addressed, and I added suggestion on how to check for aliases hiding real board names.

pabigot and others added 2 commits April 7, 2020 09:44
Zephyr board names must be globally unique which requires that they
encode all necessary information to identify a specific target.
Typing in these names can be inconvenient to developers working on
multiple targets within a single workspace.

Extend the cmake infrastructure to read an optional board aliases file
that will map custom aliases to the corresponding canonical Zephyr
board name.

Signed-off-by: Peter A. Bigot <pab@pabigot.com>
This commit will print a CMake notice if a user defines a board alias
that is identical to an existing board name.

Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
@pabigot pabigot force-pushed the pabigot/20200404a branch from 578dd9b to 5410f97 Compare April 7, 2020 14:45
@pabigot
Copy link
Collaborator Author

pabigot commented Apr 7, 2020

@tejlmand Thanks. If these changes are not acceptable please say so, otherwise I think this is ready.

I've taken that with a couple tweaks, based on the documentation for message modes saying that the default NOTICE level is for "Important message printed to stderr to attract user’s attention":

  • I've downgraded the alias message to STATUS from default NOTICE, as the user created the aliases so the substitution is expected and should not be highlighted.
  • I've downgraded the diagnostic on overrides from WARNING to NOTICE, since it's worth drawing attention to this but again it should be expected and not produce a warning.

Output now is:

tirzah[51]$ rm -rf build && west build -b frdm_k64f
-- west build: generating a build system
Including boilerplate (Zephyr base): /mnt/devel/external/zp/zephyr/cmake/app/boilerplate.cmake
-- Application: /mnt/devel/external/zp/zephyr/samples/subsys/fs/littlefs
-- Zephyr version: 2.2.99 (/mnt/devel/external/zp/zephyr)
-- Found PythonInterp: /usr/bin/python3 (found suitable version "3.7.5", minimum required is "3.6") 
-- Board: frdm_k64f
-- Aliased BOARD=frdm_k64f changed to nrf52840dk_nrf52840
Board alias frdm_k64f is hiding the real board of same name
-- Found west: /home/pab/.local/bin/west (found suitable version "0.7.1", minimum required is "0.7.1")
-- Found toolchain: zephyr (/usr/local/zephyr-sdk)
-- Found BOARD.dts: /mnt/devel/external/zp/zephyr/boards/arm/nrf52840dk_nrf52840/nrf52840dk_nrf52840.dts
-- Found devicetree overlay: /mnt/devel/external/zp/zephyr/samples/subsys/fs/littlefs/boards/nrf52840dk_nrf52840.overlay
...

@tejlmand
Copy link
Collaborator

tejlmand commented Apr 8, 2020

I've downgraded the alias message to STATUS from default NOTICE, as the user created the aliases so the substitution is expected and should not be highlighted.
I've downgraded the diagnostic on overrides from WARNING to NOTICE, since it's worth drawing attention to this but again it should be expected and not produce a warning.

I'm happy with that as well.
It was just a fast and simple suggestion.

So still approved.

@carlescufi carlescufi merged commit b6c18dd into zephyrproject-rtos:master Apr 9, 2020
@pabigot pabigot deleted the pabigot/20200404a branch April 20, 2020 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants