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

Deprecate set_conf_file #13948

Merged

Conversation

SebastianBoe
Copy link
Collaborator

In-tree, all usage of 'set_conf_file' is implementing the same
pattern (with some inconsequential differences).

To reduce copy-paste and get closer to dropping support for
'set_conf_file', we include this pattern into the built-in set of
patterns.

'set_conf_file' is causing issues for future extensions,
e.g. multi-image support, and is also the source of a lot of
duplicated code.

Note support for 'set_conf_file' is not dropped, it is only marked as
deprecated.

In-tree, all usage of 'set_conf_file' is implementing the same
pattern (with some inconsequential differences).

To reduce copy-paste and get closer to dropping support for
'set_conf_file', we include this pattern into the built-in set of
patterns.

'set_conf_file' is causing issues for future extensions,
e.g. multi-image support, and is also the source of a lot of
duplicated code.

Note support for 'set_conf_file' is not dropped, it is only marked as
deprecated.

Signed-off-by: Sebastian Bøe <[email protected]>
Port all users of 'set_conf_file' to use the built-in rules
instead. This follows the convention-over-configuration principle to
make the system as a whole simpler and more consistent.

Signed-off-by: Sebastian Bøe <[email protected]>
@SebastianBoe
Copy link
Collaborator Author

@pfalcon : FYI

Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

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

Indeed, it is just useless to replicate the same code all over the place. So +1 for the change. There are some unrelated changes introduced in this PR, those should be removed and possibly submitted in a separate PR/commit.

include($ENV{ZEPHYR_BASE}/samples/net/common/common.cmake)
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated change

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True, but, we should have a newline at the end of the file.

I can create another commit for this, but I would prefer to leave it as-is.

Copy link
Member

Choose a reason for hiding this comment

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

Adding a new commit into this set should not be a big issue. Don't you need to remove the extra CONFIG_I2C_VIRTUAL settings anyway?

@@ -2,3 +2,4 @@ CONFIG_I2C_STM32_V2=y
CONFIG_I2C_STM32_INTERRUPT=y
CONFIG_I2C_1=y
CONFIG_I2C_2=y
CONFIG_I2C_VIRTUAL=n
Copy link
Member

Choose a reason for hiding this comment

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

unrelated change

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, it is related.

Unlike all other samples, i2c_slave_api had two different files for the "common" configuration.

One that was used when no board configuration was needed (prj_virtual.conf), and another that is combined with board config (prj_base.conf).

This pattern is not supported by boilerplate.cmake, so this sample was changed to only have one "common" config file (prj.conf). Doing so required modifying the board configuration files to disable some of the "common" configuration from prj_base.conf.

Copy link
Member

Choose a reason for hiding this comment

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

Ok.

@@ -2,3 +2,4 @@ CONFIG_I2C_STM32_V2=y
CONFIG_I2C_STM32_INTERRUPT=y
CONFIG_I2C_1=y
CONFIG_I2C_2=y
CONFIG_I2C_VIRTUAL=n
Copy link
Member

Choose a reason for hiding this comment

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

unrelated change

@codecov-io
Copy link

Codecov Report

Merging #13948 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13948      +/-   ##
==========================================
- Coverage   52.29%   52.25%   -0.04%     
==========================================
  Files         307      307              
  Lines       45429    45429              
  Branches    10506    10506              
==========================================
- Hits        23756    23740      -16     
- Misses      16887    16899      +12     
- Partials     4786     4790       +4
Impacted Files Coverage Δ
subsys/testsuite/ztest/include/ztest_assert.h 38.88% <0%> (-38.89%) ⬇️
subsys/testsuite/ztest/src/ztest.c 65.28% <0%> (-7.44%) ⬇️

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 e731bdc...fdee915. Read the comment docs.

Copy link
Contributor

@dbkinder dbkinder left a comment

Choose a reason for hiding this comment

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

+1 for doc changes

@nashif nashif merged commit 4e5300b into zephyrproject-rtos:master Mar 1, 2019
@SebastianBoe SebastianBoe deleted the deprecate_set_conf_file branch March 1, 2019 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants