cmake: Introduce a user-local configuration file #9801

SebastianBoe · 2018-09-05T12:57:01Z

Introduce a .cmake file that is included by CMake to support
machine-specific configuration without using environment
variables (except for ZEPHYR_BASE).

User testing is showing that Windows users are running into a myriad
of user errors related to environment variables[0]. This PR intends to
resolve them by limiting the required use of environment variables to
a minimum; just ZEPHYR_BASE.

The user errors that will be resolved include:

Forgetting to run zephyr-env.cmd when opening a new terminal.

Not realizing that the environment variable has not been set on all
alive terminals.

And finally; not cleaning the build directory that contains a
prioritized cached value.

The configuration file needs a location and a format.

CMake was chosen for the format because it allows complex
configuration, such as modal configuration and changing the
configuration based on which ZEPHYR_BASE repo is active.

The file location was chosen to be outside of the Zephyr repository to
avoid accidental git cleaning.

[0] #9578

Signed-off-by: Sebastian Bøe [email protected]

nashif · 2018-09-05T12:59:23Z

Forgetting to run zephyr-env.cmd when opening a new terminal.

Not realizing that the environment variable has not been set on all
alive terminals.

those are not specific to windows...

nashif

this remove flexibility we currently have switching between different toolchains and tools and forces us to set those per project to override some defaults maintained in a configuration files.

nashif · 2018-09-05T13:02:45Z

doc/application/application.rst

@@ -382,7 +382,8 @@ again.
 .. note:: The ``run`` target will use the QEMU binary available from the Zephyr
          SDK by default. To use an alternate version of QEMU, for example the
          version installed on your host or a custom version, set the
-          environment variable ``QEMU_BIN_PATH`` to the alternate path.


thats defeats the purpose, if I have this in a file, then it will always be used. What we want is being able to switch between different versions of Qemu based on the session.

nashif · 2018-09-05T13:03:38Z

cmake/local_template.cmake

@@ -0,0 +1,4 @@
+# set(ZEPHYR_SDK_INSTALL_DIR $ENV{HOME}/zephyr-sdk)
+# set(ZEPHYR_TOOLCHAIN_VARIANT zephyr)


and how do you switch between variants, this is something I do on daily basis.

With bash I have

sebo@mach:~$ ls .zephyr*
.zephyr_clang.sh .zephyr_esp.sh .zephyr_issm.sh .zephyrrc .zephyr_sdk.sh .zephyr_xtools.sh
.zephyr_cross_compile.sh .zephyr_gccarmemb.sh .zephyr_kbuild.sh .zephyr_riscv32.sh .zephyr_xcc.sh

today to switch between variants.

with the new scheme you could switch between variants in a session like this:

local.cmake:

if($ENV{zt} STREQUAL gccarmemb) set(GCCARMEMB_TOOLCHAIN_PATH /home/sebo/Downloads/gcc-arm-none-eabi-7-2018-q2-update/) set(ZEPHYR_TOOLCHAIN_VARIANT gccarmemb) else() set(ZEPHYR_SDK_INSTALL_DIR $ENV{HOME}/zephyr-sdk) set(ZEPHYR_TOOLCHAIN_VARIANT zephyr) endif() set_ifndef(BOARD nrf52_pca100401)

sebo@mach:~$ export zt=gccarmemb sebo@mach:~$ cmake ..

With bash I have

sebo@mach:~$ ls .zephyr*
.zephyr_clang.sh .zephyr_esp.sh .zephyr_issm.sh .zephyrrc .zephyr_sdk.sh .zephyr_xtools.sh
.zephyr_cross_compile.sh .zephyr_gccarmemb.sh .zephyr_kbuild.sh .zephyr_riscv32.sh .zephyr_xcc.sh

and all I have to do is just set the correct variant and maintain all related env variables in ~/.zephyrrc

this will also most likely break how sanitycheck works btw.

and all I have to do is just set the correct variant and maintain all related env variables in ~/.zephyrrc

This use-case will still be supported, just using CMake code instead of bash.

this will also most likely break how sanitycheck works btw.

I wish to deprecate and discourage the use of zephyr-env.sh, but not remove it (for compatibility reasons), so sanitycheck will not be broken by this change.

I wish to deprecate and discourage the use of zephyr-env.sh

why? All that does is set ZEPHYR_BASE, you will have to export the variable using some other mean and the argument of forgetting to do that will still apply, i.e. someone might forget to set ZEPHYR_BASE in the new terminal.

sanitycheck does not depend on zephyr-env.sh btw, but it does depend on the environment variables.

Because there are fewer usability issues[0] when users set ZEPHYR_BASE and maintain a local.cmake file than when they run zephyr-env.cmd and maintain a .zephyrrc file.

[0] The three usability issues are described in the commit message. True, they are not resolved for ZEPHYR_BASE, but fewer env vars means fewer user-error opportunities.

SebastianBoe · 2018-09-05T13:30:14Z

this remove flexibility we currently have switching between different toolchains and tools and forces us to set those per project to override some defaults maintained in a configuration files.

I might be misunderstanding you, but I believe this does not remove this flexibility, see comment #9801 (comment)

nashif · 2018-09-05T13:33:55Z

cmake/app/boilerplate.cmake

+  # repo.
+  set(local_dir ${ZEPHYR_BASE})
+endif()
+set(local_cmake ${local_dir}/zephyr/local.cmake)


Is that file a user or a system/application file? AFAIK configuration files that are maintained by the user are commonly maintained as .rc files and if there are many, they will be in some .rc/ directory. If this is going to .local/ they might appear as non-user editable and do not belong to the user.

This is a user file.

I was not aware of the .rc convention, I will look into if it makes sense to use it here.

Using the term 'rc' would communicate something appropriate about the file, but I don't want to use the suffix .rc as that miscommunicates something about the format of the file. So if we are to use the term 'rc' I would suggest rc.cmake or local.rc.cmake.

AFAIK configuration files that are maintained by the user are commonly maintained as .rc files and if there are many, they will be in some .rc/ directory. If this is going to .local/ they might appear as non-user editable and do not belong to the user.

AFAIK placing configuration files in ~/.config/$VENDOR is an appropriate location and will have the appropriate permissions. (Initially I supported XDG_CONFIG_HOME in addition to HOME/.config, but found that this complicated things for the user as there were two locations to search for the file. I believe it is safe to not support XDG_CONFIG_HOME as there are so many other applications that also do not support it).

codecov-io · 2018-09-05T13:57:30Z

Codecov Report

Merging #9801 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #9801   +/-   ##
=======================================
  Coverage   52.51%   52.51%           
=======================================
  Files         213      213           
  Lines       26082    26082           
  Branches     5624     5624           
=======================================
  Hits        13696    13696           
  Misses      10135    10135           
  Partials     2251     2251

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0329ec7...ba089f6. Read the comment docs.

If one forgets to source the zephyr-env.sh script before starting a doc build (using the top-level Makefile), a long screenful of error messages go by and you can miss what the actual problem is. Check if ZEPHYR_BASE is not set and give a nice short error message. (Updated to a more generic error message in anticipation of #9801 deprecating use of zephyr-env.sh) Signed-off-by: David B. Kinder <[email protected]>

SebastianBoe · 2018-09-06T13:27:27Z

@nashif : I believe that all review feedback has been addressed, please re-assess when convenient.

carlescufi · 2018-09-06T14:43:17Z

@SebastianBoe just got to taking a look at this, but before anything else, how does this relate to the current ~/.zephyrrc (Unix) and %userprofile%\zephyrrc.cmd on Windows? Don't those achieve the same purpose today, albeit with different semantics?

My %userprofile%\zephyrrc.cmd:

set ZEPHYR_TOOLCHAIN_VARIANT=gnuarmemb
set GNUARMEMB_TOOLCHAIN_PATH=c:\gccarmemb

SebastianBoe · 2018-09-07T07:23:09Z

It is nearly the same, except for the syntax and the fact that the user does not have to execute a script to to ensure that the config file is used.

So there are fewer opportunities for user-error.

EDIT: Another difference, is that if the user does not use zephyrrc.cmd / zephyrrc there will be 1 fewer language necessary to learn to use Zephyr. Which makes it easier to use Zephyr.

carlescufi · 2018-09-07T11:47:09Z

@SebastianBoe understood.

That said users still need to know how to export ZEPHYR_BASE, so the point about one less language is a bit moot.

That said I am in favor of this change. I am going to add a couple more people to this review to get more feedback.

carlescufi · 2018-09-07T11:47:51Z

@mbolivar @MaureenHelm would like to hear your opinion on this proposal.

MaureenHelm · 2018-09-07T15:11:48Z

Forgetting to run zephyr-env.cmd when opening a new terminal.

It's a common mistake and I like that you're fixing it.

My %userprofile%\zephyrrc.cmd:

set ZEPHYR_TOOLCHAIN_VARIANT=gnuarmemb
set GNUARMEMB_TOOLCHAIN_PATH=c:\gccarmemb

I also have:

set PATH=%PATH%;C:\Program Files (x86)\SEGGER\JLink_V631d

I think this change means I'll have to set the JLink path elsewhere, right?

nashif · 2018-09-07T18:07:10Z

Forgetting to run zephyr-env.cmd when opening a new terminal.

I not sure I understand what we are fixing here, you still have to set ZEPHYR_BASE somehow, whether you do that manually, using a script, you might forget it, and this is not specific to windows at all.

I do not think this is a reason enough to change the behaviour and move this to cmake, environment variables provide lots of flexibility that many will not want to give away. So I am still a -1 on this.

If one forgets to source the zephyr-env.sh script before starting a doc build (using the top-level Makefile), a long screenful of error messages go by and you can miss what the actual problem is. Check if ZEPHYR_BASE is not set and give a nice short error message. (Updated to a more generic error message in anticipation of zephyrproject-rtos#9801 deprecating use of zephyr-env.sh) Signed-off-by: David B. Kinder <[email protected]>

SebastianBoe · 2018-09-10T07:02:02Z

I not sure I understand what we are fixing here,

Which of the three listed user-errors are unclear? Perhaps I can
improve the explanation.

you still have to set ZEPHYR_BASE somehow, whether you do that
manually, using a script, you might forget it, and this is not
specific to windows at all.

Correct. This PR does not address any user-errors related to
ZEPHYR_BASE, it concerns only the configuration that
traditionally goes into zephyr-env.cmd.

I do not think this is a reason enough to change the behaviour
and move this to cmake, environment variables provide lots of
flexibility that many will not want to give away. So I am still a
-1 on this.

We are not removing the ability to use environment variables. We
are removing some of the environment variable documentation
though, if this approach is accepted I could try to find some way
to explain that configuration can be maintained in env vars as
well.

EDIT: To make this less theoretical, and more concrete, have a look
at the user errors discovered in a recent workshop. Interestingly, this PR
resolves all three of them. Meaning, if this PR were merged, those users
would have been able to get started without issues.
#9578

dbkinder · 2018-09-11T20:36:29Z

doc/getting_started/getting_started.rst

@@ -30,6 +32,20 @@ using Git to clone the repository anonymously. Enter:
 You have successfully checked out a copy of the source code to your local
 machine in a ``zephyr`` folder in your home directory.

+The environment variable ``ZEPHYR_BASE`` must be set to the repository


Let's simplify this to:

Zephyr development tools assume the environment variable ``ZEPHYR_BASE`` is set to the repository path. Here are examples for setting this variable on our supported development platforms:

dbkinder · 2018-09-11T20:43:52Z

doc/getting_started/getting_started.rst

+should be created to manage user-specific configurations such as
+install paths.
+
+It is a CMake build script that is executed early in the configuration


Change to:

A CMake build script is used early in the configuration process to replace all uses of environment variables, except for ``ZEPHYR_BASE``. The following sections define where this :file:`local.cmake` file lives and what to put in it. The :file:`local.cmake` file location depends on the platform:

dbkinder · 2018-09-11T20:46:32Z

doc/getting_started/getting_started.rst

-the ZEPHYR_SDK_INSTALL_DIR environment variable must be unset
+As already noted above, the SDK also includes prebuilt host tools. To
+use the SDK's prebuilt host tools alongside a 3rd party or custom
+cross-compiler, keep the ZEPHYR_SDK_INSTALL_DIR variable set to the


Add the double back-ticks on the environment variable name:

``ZEPHYR_SDK_INSTALL_DIR``

dbkinder · 2018-09-11T20:48:23Z

doc/getting_started/getting_started.rst

-environment variable set to the Zephyr SDK installation directory.
-See `Set Up the Development Environment`_ for more details on the
-ZEPHYR_SDK_INSTALL_DIR environment variable.
+party cross compiler. To do that, keep ZEPHYR_SDK_INSTALL_DIR set to


Add the double back-ticks on the environment variable name:

``ZEPHYR_SDK_INSTALL_DIR``

dbkinder · 2018-09-11T20:50:51Z

doc/getting_started/installation_linux.rst

@@ -176,12 +171,10 @@ Follow these steps to install the SDK on your Linux host system.
  To use the same toolchain in new sessions in the future, you can set the
  variables in the file :file:`${HOME}/.zephyrrc`, for example:

-  .. code-block:: console
+   .. code-block:: CMake


don't indent the code-block directive

dbkinder · 2018-09-11T20:52:17Z

doc/getting_started/installation_win.rst

-
-      cd %userprofile%\zephyr
-      pip3 install -r scripts/requirements.txt
+      pip3 install -r %ZEPHYR_BASE%/scripts/requirements.txt


we lost the code-block directive in your edit.

Introduce a .cmake file that is included by CMake to support machine-specific configuration without using environment variables (except for ZEPHYR_BASE). User testing is showing that Windows users are running into a myriad of user errors related to environment variables[0]. This PR intends to resolve them by limiting the required use of environment variables to a minimum; just ZEPHYR_BASE. The user errors that will be resolved include: Forgetting to run zephyr-env.cmd when opening a new terminal. Not realizing that the environment variable has not been set on all alive terminals. And finally; not cleaning the build directory that contains a prioritized cached value. The configuration file needs a location and a format. CMake was chosen for the format because it allows complex configuration, such as modal configuration and changing the configuration based on which ZEPHYR_BASE repo is active. The file location was chosen to be outside of the Zephyr repository to avoid accidental git cleaning. [0] zephyrproject-rtos#9578 Signed-off-by: Sebastian Bøe <[email protected]>

SebastianBoe · 2018-09-12T07:00:05Z

@dbkinder : Thank you for the feedback, all points are now addressed.

aescolar

I agree with @nashif points
+
You comment that:

EDIT: Another difference, is that if the user does not use zephyrrc.cmd / zephyrrc there will be 1 fewer language necessary to learn to use Zephyr. Which makes it easier to use Zephyr.

I think it is more reasonable to assume a user knows how to set an environment variable in his shell of choice, than to know about cmake's scripts.
If anything, you are adding another language. And not one many know. I think the assumption should be that typical users do not need to mess with any build scripts, but just call them.
+

The file location was chosen to be outside of the Zephyr repository to avoid accidental git cleaning.

Why not add a line in .gitignore for it? I really dislike having Zephyr "install" itself around. Soon Zephyr will need to come with an uninstall program..

SebastianBoe · 2018-09-13T07:38:38Z

I think it is more reasonable to assume a user knows how to set an environment variable in his shell of choice, than to know about cmake's scripts.

That is not a reasonable assumption. Most embedded windows developers do not know how to write bat files.

I am not adding a language. CMake is already required knowledge in Zephyr. Typical users must write application build scripts, not only read them.

Why not add a line in .gitignore for it? I really dislike having Zephyr "install" itself around. Soon Zephyr will need to come with an uninstall program..

gitignored files can be accidentally deleted by git clean.

aescolar · 2018-09-13T07:47:06Z

Typical users must write application build scripts, not only read them.

I would expect typical users to work in a team of several developers, where only 1 of them messes with the build scripts, and the rest at best add the odd source file to the library (if the library wasn't set to just pick *.c like many of our samples do)

SebastianBoe · 2018-09-13T10:03:24Z

I am sure that users familiar or not with CMake syntax will be able to follow the documentation. An excerpt of the documentation:

#. Enable the compiler by creating or modifying the file
   :ref:`local.cmake <getting_started_local_cmake>` with:

   .. code-block:: CMake

       # On Linux/macOS	      

set(ZEPHYR_TOOLCHAIN_VARIANT gnuarmemb)
set(GNUARMEMB_TOOLCHAIN_PATH $ENV{HOME}/gcc-arm-none-eabi-7-2018-q2-update/)

	      # On Windows

set(ZEPHYR_TOOLCHAIN_VARIANT gnuarmemb)
set(GNUARMEMB_TOOLCHAIN_PATH $ENV{userprofile}\gcc-arm-none-eabi-7-2018-q2-update\)

dbkinder

+1 for doc changes, thanks.

SebastianBoe · 2018-11-08T09:26:48Z

Closing as this was not able to get through review.

SebastianBoe requested review from dbkinder and nashif as code owners September 5, 2018 12:57

SebastianBoe requested a review from carlescufi September 5, 2018 12:57

SebastianBoe added this to the v1.14.0 milestone Sep 5, 2018

SebastianBoe added Feature A planned feature with a milestone area: Build System labels Sep 5, 2018

nashif requested changes Sep 5, 2018

View reviewed changes

SebastianBoe force-pushed the local_configuration_file branch from 57086d3 to 78b6f3f Compare September 5, 2018 13:18

nashif reviewed Sep 5, 2018

View reviewed changes

carlescufi requested review from mbolivar and MaureenHelm September 7, 2018 11:47

dbkinder suggested changes Sep 11, 2018

View reviewed changes

SebastianBoe force-pushed the local_configuration_file branch from 78b6f3f to ba089f6 Compare September 12, 2018 06:59

aescolar requested changes Sep 13, 2018

View reviewed changes

dbkinder approved these changes Sep 14, 2018

View reviewed changes

SebastianBoe mentioned this pull request Oct 12, 2018

Can we get rid of zephyr-env.sh? #10534

Closed

SebastianBoe closed this Nov 8, 2018

SebastianBoe mentioned this pull request Feb 1, 2019

out-of-installation use of west zephyrproject-rtos/west#175

Closed

		@@ -0,0 +1,4 @@
		# set(ZEPHYR_SDK_INSTALL_DIR $ENV{HOME}/zephyr-sdk)
		# set(ZEPHYR_TOOLCHAIN_VARIANT zephyr)

cmake: Introduce a user-local configuration file #9801

cmake: Introduce a user-local configuration file #9801

Conversation

SebastianBoe commented Sep 5, 2018

nashif commented Sep 5, 2018

nashif left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianBoe commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 5, 2018 • edited Loading

Codecov Report

SebastianBoe commented Sep 6, 2018

carlescufi commented Sep 6, 2018

SebastianBoe commented Sep 7, 2018 • edited Loading

carlescufi commented Sep 7, 2018

carlescufi commented Sep 7, 2018

MaureenHelm commented Sep 7, 2018

nashif commented Sep 7, 2018

SebastianBoe commented Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianBoe commented Sep 12, 2018

aescolar left a comment

Choose a reason for hiding this comment

SebastianBoe commented Sep 13, 2018

aescolar commented Sep 13, 2018

SebastianBoe commented Sep 13, 2018

dbkinder left a comment

Choose a reason for hiding this comment

SebastianBoe commented Nov 8, 2018

codecov-io commented Sep 5, 2018 •

edited

Loading

SebastianBoe commented Sep 7, 2018 •

edited

Loading

SebastianBoe commented Sep 10, 2018 •

edited

Loading