-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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: Extracted Zephyr module processing into python script #14667
cmake: Extracted Zephyr module processing into python script #14667
Conversation
All checks are passing now. Review history of this comment for details about previous failed status. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am uneasy about this. While parsing the modules with Python seems like a good idea, I am not sure about generating CMake files from Python. This is a bit extreme, isn't it? Wouldn't it be better to print a list of paths to CMakeLists.txt
from zephyr_modules.py
and then have CMake parse that output and call add_subdirectory()
on each?
scripts/zephyr_module.py
Outdated
with open(module_yml, 'r') as f: | ||
meta = yaml.safe_load(f.read()) | ||
|
||
schema = yaml.safe_load(METADATA_SCHEMA) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might need a Loader=
param to avoid warnings with newer versions of PyYaml: zephyrproject-rtos/ci-tools@a529866
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, will add.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a look at: zephyrproject-rtos/ci-tools@a529866
That code uses yaml.load
and not yaml.safe_load
as this code.
So that is not an issue here (as well as safe_load doesn't support the Loader
argument)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also feels a bit strange to parse this schema for each module, since it isn't changing. It could either be a global variable or passed as an argument instead to avoid that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbolivar fixed
Codecov Report
@@ Coverage Diff @@
## master #14667 +/- ##
=======================================
Coverage 52.92% 52.92%
=======================================
Files 309 309
Lines 45268 45268
Branches 10451 10451
=======================================
Hits 23956 23956
Misses 16544 16544
Partials 4768 4768 Continue to review full report at Codecov.
|
Thanks for the comments.
It was done this way, as to have same principle used for both Kconfig and CMake. Secondly, as commented by @SebastianBoe output from scripts should ideally go through a file, and thus, instead of generating text file with a list of folders to include, which then must be parsed by CMake, then I decided to generate the CMake file directly. Third, if we in future needs to create additional type of files that can be included, e.g. DTS, then that can be supported with The
as mentioned, printing directly from scripts is discouraged, so would need to go through a file anyway. |
You don't have to print from a script to do this. You can have the python script write a file that is easily parse-able by CMake, e.g csv, or lines of set(FOO bar). Generally, it is ok to generate data, but not code. There is a fine line between them, but e.g. add_subdirectory looks like code to me. |
0c3e30f
to
21a2e64
Compare
Yes, that's exactly what i'm stating.
And to that I replied a file should be generated from the Python script in any case, just as you also state.
As does the Kconfig generated file: I think the Remember, the python script is now generic, so that it can be used outside CMake, e.g. for CI. |
Yeah, but that's unavoidable AFAICT. But for CMake, we are able to avoid more code generation than in the current proposal AFAICT. |
Yes, this is preferable. |
I see that @SebastianBoe aligns with my suggestion. As he mentions, for |
21a2e64
to
0e50cb3
Compare
Fixed. |
scripts/zephyr_module.py
Outdated
if section is not None: | ||
cmake_path = section.get('cmake', cmake_path) | ||
kconfig_file = section.get('kconfig', kconfig_file) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should assert when the yaml file is pointing to a non-existent file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're referring to using try statements to open the file, and doing something like calling sys.exit
or printing to stderr in the exception handler, right? Not an actual assert
statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SebastianBoe yep, see your point, although I agree with @mbolivar you don't want assert, but instead raise an exception in order to abort with error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually I like stack traces, but I see this would be a user-input error through misconfigured .yml, so this should be a message through stderr and an exit code as @mbolivar says.
Internal system error -> stack trace.
User-input error -> error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
scripts/zephyr_module.py
Outdated
help='List of extra modules to parse') | ||
args = parser.parse_args() | ||
|
||
if args.kconfig_out is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just
if args.kconfig_out:
is best-practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that if foo
and if not bar
for the current set of arguments is more readable. But this is not a universal best practice.
If there were any arguments with type=int
then you would need to explicitly check for None
if you want to test "argument not provided", to avoid issues when the value is given, just zero:
#!/usr/bin/env python3
import argparse
parser = argparse.ArgumentParser()
parser.add_argument('--foo', type=int)
args = parser.parse_args()
if args.foo:
print('foo!')
else:
print('no foo :(')
E.g.:
$ ./foo.py
no foo :(
$ ./foo.py --foo 1
foo!
$ ./foo.py --foo 0
no foo :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that is best practice, also PEP8 states:
https://www.python.org/dev/peps/pep-0008/#id51
Comparisons to singletons like None should always be done with is or is not, never the equality operators.
Also, beware of writing if x when you really mean if x is not None
And here, I do indeed mean is not None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here, I do indeed mean is not None.
But we want to express "x is not None and x != empty_string", which we will get with "if x", but we don't get with a None comparison.
Allowing x == empty_string will cause weird issues further down the line AFAICT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbolivar : I agree, we wouldn't want to use "if x" for the int type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SebastianBoe you're right, haven't consider that a user could write --cmake-out=
without providing a filename in which case the value would indeed be an empty string.
So that is fixed now.
scripts/zephyr_module.py
Outdated
type: str | ||
''' | ||
|
||
AUTO_GENERATED_HEADER = '# NOTE: THIS FILE IS AUTOGENERATED - DO NOT EDIT\n\n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we drop this?
Then we can drop the regex:
Users really should know that they can't modify any files in the build directory ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine with me, but it was originally introduced due to this comment: #13067 (comment)
- the filename should reflect that, e.g. modules.generated.kconfig
- Add a header of "# NOTE: THIS IS AUTOGENERATED BY " to this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree with that comment.
Most files in the build directory are generated (as opposed to copied from source, which is rarely done in CMake-based build systems). And most files do not have the word generated in the filename or in the header.
But I can understand having the disclaimer in generated C code for users with editors that don't make it obvious that they have opened a file in the build directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree with that comment.
Personally this is not important to me, and per default I would not have put it in first place.
I was just referring to why it was there.
So it's removed again.
scripts/zephyr_module.py
Outdated
def generate_file_header(includefile): | ||
path = os.path.dirname(includefile) | ||
if path != '' and not os.path.exists(path): | ||
os.makedirs(path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be dropped, the user should be able to make the directory of the path he inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the user
in this case is CMake, and at this point in time, then the folder ${PROJECT_BINARY_DIR}
doesn't exist.
The reason this worked in CMake was because file(write <path>/filename)
will automatically create <path>
, if it doesn't exists.
${PROJECT_BINARY_DIR}
is used because that variable is already exported to Kconfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CMake should create the directory then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Folder creation moved to CMake.
scripts/zephyr_module.py
Outdated
path = os.path.dirname(includefile) | ||
if path != '' and not os.path.exists(path): | ||
os.makedirs(path) | ||
if includefile is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if-statement is always true so it can be dropped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep. has become obsolete when I refactored code.
Thanks for spotting.
scripts/zephyr_module.py
Outdated
if path != '' and not os.path.exists(path): | ||
os.makedirs(path) | ||
if includefile is not None: | ||
with open(includefile, 'w+') as f: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the '+' can be dropped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree.
Fixed
scripts/zephyr_module.py
Outdated
os.makedirs(path) | ||
if includefile is not None: | ||
with open(includefile, 'w+') as f: | ||
f.write(AUTO_GENERATED_HEADER) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming you agree about https://github.com/zephyrproject-rtos/zephyr/pull/14667/files#r266966468 we can just 'pass' here to create the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be a bit weird. If the entire point of this function is to generate the header, and that is not needed, we could just delete the function and move the with statement to a place in the file where the contents are available or generated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah. as I wrote #14667 (comment)
it seems people have different opinions about headers.
Personally I agree with @SebastianBoe that when files are in build folder, they should be assumed to be generated.
But if people wants a header, that also fine with me.
Purpose of code is correctly to generate the header (and folder if missing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as it seems we agree to not have the header in files then that code is now removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some comments about style since I guess this won't be merged for v1.14 and we are thus in no hurry and can discuss what's Pythonic or not at our leisure ;).
scripts/zephyr_module.py
Outdated
Process a list of projects and create Kconfig / CMake include files for \ | ||
projects which are also a Zephyr module''') | ||
|
||
parser.add_argument('--kconfig-out', required=False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
False is the default for required when the argument is optional, so you could just drop those kwargs entirely, here and below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true, and completely agree.
This was simply reuse of the old code from yaml_to_cmake.py, where the output arg was required=True
Should have deleted instead of changing to False
scripts/zephyr_module.py
Outdated
help='List of extra modules to parse') | ||
args = parser.parse_args() | ||
|
||
if args.kconfig_out is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that if foo
and if not bar
for the current set of arguments is more readable. But this is not a universal best practice.
If there were any arguments with type=int
then you would need to explicitly check for None
if you want to test "argument not provided", to avoid issues when the value is given, just zero:
#!/usr/bin/env python3
import argparse
parser = argparse.ArgumentParser()
parser.add_argument('--foo', type=int)
args = parser.parse_args()
if args.foo:
print('foo!')
else:
print('no foo :(')
E.g.:
$ ./foo.py
no foo :(
$ ./foo.py --foo 1
foo!
$ ./foo.py --foo 0
no foo :(
scripts/zephyr_module.py
Outdated
os.makedirs(path) | ||
if includefile is not None: | ||
with open(includefile, 'w+') as f: | ||
f.write(AUTO_GENERATED_HEADER) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be a bit weird. If the entire point of this function is to generate the header, and that is not needed, we could just delete the function and move the with statement to a place in the file where the contents are available or generated.
scripts/zephyr_module.py
Outdated
|
||
def main(): | ||
parser = argparse.ArgumentParser(description=''' | ||
Process a list of projects and create Kconfig / CMake include files for \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is a backslash being used in a triple quoted string, where it is not necessary? Are you sure it's needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no it's not needed.
Wonder how it got in there in the first place.
scripts/zephyr_module.py
Outdated
meta = yaml.safe_load(f.read()) | ||
|
||
schema = yaml.safe_load(METADATA_SCHEMA) | ||
pykwalify.core.Core(source_data=meta, schema_data=schema).validate() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth putting this in a try statement and aborting with a clean error if it fails, instead of just printing a stack trace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
guess moving code around catches you attention ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now catching a printing more informative message to user.
scripts/zephyr_module.py
Outdated
val_str = '{}:{}\n'.format(os.path.basename(module), cmake_path_abs) | ||
|
||
with open(cmake_out, 'a') as f: | ||
f.write(val_str) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a bit strange to do it this way. Why not just assume cmake_out
is a file-like argument if it is not None, and open it up once for appending in the loop that calls this function?
Then this could just be:
if os.path.isfile(cmake_file_abs) and cmake_out is not None:
print('{}:{}'.format(os.path.basename(module), cmake_path_abs), file=cmake_out)
It should be a bit faster as well, since the program won't be opening and closing the file every time a line is added.
If it's important that it's just a \n
and not a CRLF on Windows, then using end='\n'
in the call to print
would make sure of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, good point.
Guess it's just a personal preference for the
with open(<file>) as f:
f.write(<something>)
As you avoid the close, as well as the try-catch for the IO handling.
Although, I do agree, that only opening the file once for writing is a small optimization.
scripts/zephyr_module.py
Outdated
if os.path.isfile(kconfig_file_abs) and kconfig_out is not None: | ||
val_str = 'osource "{}"\n\n'.format(kconfig_file_abs) | ||
|
||
with open(kconfig_out, 'a') as f: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment for the way cmake_out
is handled, above, applies here.
scripts/zephyr_module.py
Outdated
with open(module_yml, 'r') as f: | ||
meta = yaml.safe_load(f.read()) | ||
|
||
schema = yaml.safe_load(METADATA_SCHEMA) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also feels a bit strange to parse this schema for each module, since it isn't changing. It could either be a global variable or passed as an argument instead to avoid that.
scripts/zephyr_module.py
Outdated
if section is not None: | ||
cmake_path = section.get('cmake', cmake_path) | ||
kconfig_file = section.get('kconfig', kconfig_file) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're referring to using try statements to open the file, and doing something like calling sys.exit
or printing to stderr in the exception handler, right? Not an actual assert
statement.
22ee119
to
0211794
Compare
@mbolivar @SebastianBoe all comments addressed Also removed the |
Thanks for the review |
0211794
to
678d7f4
Compare
@carlescufi @SebastianBoe @mbolivar Can you recheck, everything should be fixed :) |
e58a3bc
to
1c3ca94
Compare
7989556
to
bbe4909
Compare
Revert on next upmerge, once zephyrproject-rtos/zephyr#14667 is merged. This is required because CMAKE_CURRENT_BINARY_DIR is the same as CMAKE_BINARY_DIR upstream, but not in NCS. Signed-off-by: Carles Cufi <[email protected]>
Fixes: zephyrproject-rtos#14513 This commit move the functionality of extracting zephyr modules into generated CMake and Kconfig include files from CMake into python. This allows other tools, especially CI to re-use the zephyr module functionality. Signed-off-by: Torsten Rasmussen <[email protected]>
In order to parse all the Kconfig files that make up the whole Kconfig tree we need to process the modules, since this can optionally provide Kconfig files. In order to do so, include the modules CMake file so that they are listed and a Kconfig.modules is generated. Signed-off-by: Carles Cufi <[email protected]>
When generating the Kconfig reference documentation, enable the warnings for undefined Kconfig options in order to make sure all references are met. Signed-off-by: Carles Cufi <[email protected]>
bbe4909
to
56a77f8
Compare
Looks like this broke "hello_world" (and everything else) when not using west. I'm aware not using west is "living in the past", however I didn't really expect this at this time between -rc3 and -rc4. A meaningful error message mentioning (the lack of) west would also be nicer than a cryptic kconfig stack trace. Has the documentation been updated to clarify that west is now mandatory?
Famous last words :-) |
Kconfig.modules used to be an empty file when no modules. PR zephyrproject-rtos#14667 / commit bd7569f "cmake: Extracted Zephyr module processing into python script" unintentionally changed that to no Kconfig.modules file at all when no west and no modules. kconfig.py doesn't like that and crashes. Restore the empty file. Signed-off-by: Marc Herbert <[email protected]>
@carlescufi @tejlmand @nashif
Looks like we are trying to include module I am no expert on this, but reverting these commits seem to solve the problem. |
This is very strange. A module named "tinycbor:C" is being found by west. Can you please run the following |
Kconfig.modules used to be an empty file when no modules. PR #14667 / commit bd7569f "cmake: Extracted Zephyr module processing into python script" unintentionally changed that to no Kconfig.modules file at all when no west and no modules. kconfig.py doesn't like that and crashes. Restore the empty file. Signed-off-by: Marc Herbert <[email protected]>
Fixes: #14513
Fixes: #14989
This commit move the functionality of extracting zephyr modules into
generated CMake and Kconfig include files from CMake into python.
This allows other tools, especially CI to re-use the zephyr module
functionality.