samples: google_iot_device: Remove "command line" and replace with config.c

[pfalcon]

There is no "command line" in Zephyr RTOS. "Command line" which was used previously is an artifact of a special-purpose, testing target "native_posix", not portable to any other Zephyr target.

So, instead provide config.c file, where a user can define all needed application parameters (which previously were passed on "command line"). As such a file contain credentials, it should be never put under version control. This commit includes a template file, config.c.in, which can be copied by a user to config.c, and needed values filled in.

Signed-off-by: Paul Sokolovsky paul.sokolovsky@linaro.org

[pfalcon]

This is based on the discussion in https://github.com/atigyi/zephyr/issues/3#issuecomment-521633236 , and is a prerequisite to getting this sample running on "real" Zephyr device, like qemu_x86 (native_posix is not a real Zephyr device in that regard).

[pfalcon]

What I'd generally suggest is to completely remove this "command line" handling, i.e. remove code which is so far wrapped in #if 0 in this PR. Then, apparently rename commandline.h to config.h.

[pfalcon]

Rebased on the latest base branch.

@atigyi, This would be on critical path to https://github.com/atigyi/zephyr/issues/2, please share your feedback.

[pfalcon]

@atigyi : Ok, so I refactored this based on your comments - effectively, went full on with removing command line parsing, and replaced it with C-based config file, down to references in the code comments. As usual, tested with BOARD=native_posix.

Let me know how that looks.

[pfalcon]

@atigyi, Pushed a change based on your last round of comments (specifically, copyright header/include in config.c.in).

[atigyi]

Please check this link-error comment too.

[pfalcon]

Please check this link-error comment too.

Ah, right.

Update: don't we miss the following line in the CMakeLists.txt? configure_file(${APPLICATION_SOURCE_DIR}/src/config.c.in ${APPLICATION_SOURCE_DIR}/src/config.c)

Ok, tested. With the line added above and config.c.in filled in properly

Well, I'm not familiar with that CMake command. In my list, what's needed to be done is:

• Before building:
• Copy config.c.in to config.c
• Fill in actual connection config in config.c

Editing config.c.in directly should be discouraged - this file is under version control, so it's possible to inadvertently commit it with other changes, then push to public or otherwise not-enough-restricted repo, then we have credentials leak.

So you're right that README should be updated, working on that.

[pfalcon]

So you're right that README should be updated, working on that.

Pushed update as a separate commit, we'll yet need to squash things around later anyway.

[atigyi]

Please check this link-error comment too.

Ah, right.

Update: don't we miss the following line in the CMakeLists.txt? configure_file(${APPLICATION_SOURCE_DIR}/src/config.c.in ${APPLICATION_SOURCE_DIR}/src/config.c)

Ok, tested. With the line added above and config.c.in filled in properly

Well, I'm not familiar with that CMake command. In my list, what's needed to be done is:

• Before building:
• Copy config.c.in to config.c
• Fill in actual connection config in config.c

Editing config.c.in directly should be discouraged - this file is under version control, so it's possible to inadvertently commit it with other changes, then push to public or otherwise not-enough-restricted repo, then we have credentials leak.

So you're right that README should be updated, working on that.

First of all - and after README update - this is now clear and a good solution and I can accept it. 👍

But the .c.in suffix mislead me. .h.in / .c.in file extensions are occupied by the configure_file cmake functionality. First sentence of the description says: "Copy a file to another location and modify its contents." Actually this is the two bullet points mentioned above, but in an automated fashion (which isn't necessarily better).

And about the accidental credentials leak: a new file config.c or a modified existing file config.c.in both are a single step from git-staging (git add or git stage) and then git-commit. Maybe git add is used less frequently. But I don't see significantly larger thread against credentials leak with the manual steps.

Please just consider if this solution is better with auto-copy and fill or manual. I searched for it, no Zephyr sample uses cmake's configure_file so far but internals do.

[pfalcon]

But the .c.in suffix mislead me. .h.in / .c.in file extensions are occupied by the configure_file cmake functionality.

Ah, I see. And not exactly. ".in" suffix is a well-known convention for "template" files where things needs to be replaced to get the actual file sans ".in". Where possible, replacement happens automatically. Where not, e.g. where user credentials are needed, it needs to be copied and edited by a user. It's universal convention not tied to any tool. CMake just appear to follow well-known naming convention with its own adhoc meaning. Github gives 158 million hits when searching for "config.h.in": https://github.com/search?q=config.h.in&type=Code , and majority of those hits aren't related to CMake.

are a single step from git-staging (git add or git stage) and then git-commit. Maybe git add is used less frequently.

Well, those are 2 steps ;-). "git add" is used much less frequently. How many people process git commits is "git commit -a", which just shoves anything changed into single commit, voila. Running "git add &;lt;file>" is quite a departure from that simple course.

Please just consider if this solution is better with auto-copy and fill or manual. I searched for it, no Zephyr sample uses cmake's configure_file so far but internals do.

I think it's good idea to follow what other samples do and not rely on CMake peculiarities. CMake is quite a recent addition to Zephyr build system. And if it was changed once, it can be changed again, and then CMake-specific features will complicate migration and maintenance.

Also mind that the purpose of this patch is to get rid of the code not supported by target other than native_posix, and move on to other problem precluding it to run on other targets, and not perfectalize configuration handling. It's quite possible that when we get stuff actually working, we'll think: "Oh, we want those params to be set via Kconfig". Or maybe we'll be asked to do that during upstream code review...