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.

Sign up for GitHub

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

Jump to bottom

zephyr: Support cmake args. Add -Werror in CI #4733

Merged
merged 4 commits into from
Sep 9, 2021
Merged

zephyr: Support cmake args. Add -Werror in CI #4733

merged 4 commits into from
Sep 9, 2021

Conversation

marc-hb
Copy link
Collaborator

@marc-hb marc-hb commented Sep 8, 2021

4 commits. Main ones:

  • support CMake arguments
  • Add -Werror in CI

@marc-hb
xtensa-build-zephyr: extract new parse_args() function from main()
9e421ae 
main() was growing too big. Zero functional change.

Also rename the too generic "build()" to build_all()

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb
xtensa-build-zephyr: support passing through CMake arguments
c2e8837 
Like this:

  xtensa-build-zephyr.sh -a -- -DEXTRA_CFLAGS='-Werror -Wextra'

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb
zephyr/docker-build.sh: pass arguments through to xtensa-build-zephyr.sh
77f111c 
Allows passing compilation flags and any other argument.

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb
.github/zephyr: add -Werror
d1e612b 
Because we can. This would have caught this regression from
commit 287a5f9 ("ssp: mclk/bclk turned off unexpectedly")

-DEXTRA_CFLAGS is not very well documented but it is what Zephyr uses,
try `git -C zephyr grep -C 5 EXTRA_CFLAGS and see.

```
sof/src/drivers/intel/ssp/ssp.c: In function 'ssp_set_config_tplg':
sof/zephyr/include/sof/trace/trace.h:44:11: warning:
                   too many arguments for format [-Wformat-extra-args]
   44 |    printk("%llu: " format "\n", platform_timer_get(NULL), \
      |           ^~~~~~~~
  ...
sof/src/drivers/intel/ssp/ssp.c:763:4: note: in expansion of macro 'dai_info'
  763 |    dai_info(dai, "ssp_set_config(): hw_free stage:
                     ignore since there is still user", dai->index);
```

Using -Werror only in CI avoids slowing down developers with temporary
warnings they intend on fixing later (but before submission)

Signed-off-by: Marc Herbert <marc.herbert@intel.com>
@marc-hb
Copy link
Collaborator Author

marc-hb commented Sep 8, 2021

https://github.com/thesofproject/sof/pull/4733/checks?check_run_id=3548978888 with both -Werror and -Wextra failed as expected.

@marc-hb marc-hb marked this pull request as ready for review September 8, 2021 20:28
@marc-hb
Copy link
Collaborator Author

marc-hb commented Sep 8, 2021

No CML_SKU0955_HDA device was available in https://sof-ci.01.org/sofpr/PR4733/build10246/devicetest/. Everything else is green on that page and everywhere else (it's almost suspicious)

Zephyr builds were all successful (there's no Zephyr testing in PR at the moment, only in daily tests)

@marc-hb marc-hb mentioned this pull request Sep 8, 2021
Merged
@marc-hb marc-hb changed the title zephyr: support cmake args. Add -Werror in CI zephyr: Support cmake args. Add -Werror in CI Sep 9, 2021
This was referenced Sep 9, 2021
Closed
Closed
lgirdwood
lgirdwood approved these changes Sep 9, 2021
View reviewed changes
Copy link
Member

@lgirdwood lgirdwood left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@marc-hb what's the long term plan here ? It seem like -Werror and also --std=c99 need to be default arguments in the build (automatically defined in cmake rules). I would assume these would need to be in Zephyr cmake and be inherited by SOF cmake ? @andyross ?

@marc-hb
Copy link
Collaborator Author

marc-hb commented Sep 9, 2021 •
edited
Loading

what's the long term plan here ? It seem like -Werror and also --std=c99 need to be default arguments in the build (automatically defined in cmake rules)

I don't know about -std=c99 and Zephyr. I heard some C99 features are missing from XCC.

In general -Werror by default in the build can be UNproductive because it slows down iterative development, see one of my commit messages here. In upstream Zephyr -Werror is not in the build by default, it's added in the test suite (in twister).

Anyway this PR adds flexibility to add any flag anywhere later (and I have another, unrelated PR textually depending on it)

@lgirdwood lgirdwood merged commit 9a23191 into thesofproject:main Sep 9, 2021
@marc-hb marc-hb deleted the zephyr-cmake-args branch September 9, 2021 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lgirdwood lgirdwood lgirdwood approved these changes

@iuliana-prodan iuliana-prodan iuliana-prodan approved these changes

@cujomalainey cujomalainey Awaiting requested review from cujomalainey cujomalainey is a code owner

@dbaluta dbaluta Awaiting requested review from dbaluta dbaluta is a code owner

@zrombel zrombel Awaiting requested review from zrombel

@lyakh lyakh Awaiting requested review from lyakh lyakh is a code owner

@aiChaoSONG aiChaoSONG Awaiting requested review from aiChaoSONG

@fredoh9 fredoh9 Awaiting requested review from fredoh9

@keqiaozhang keqiaozhang Awaiting requested review from keqiaozhang

@andyross andyross Awaiting requested review from andyross

@dcpleung dcpleung Awaiting requested review from dcpleung

@kv2019i kv2019i Awaiting requested review from kv2019i kv2019i is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@marc-hb @lgirdwood @iuliana-prodan