arduino-esp32
arduino-esp32 copied to clipboard
espressif
Removes -Werror=all from cflags
Description of Change
-Werror=all creates some issues when used, which are not desirable.
Erroring out those warning-errors sometimes is too hard and not really important for Arduino users.
Most other Arduino platform do not set this cflag.
Tests scenarios
See issue #7024
Related links
Closes #7024
For example, you can try adding this flag back during CI build by creating a platform.local.txt file before building the sketches (https://arduino.github.io/arduino-cli/0.21/platform-specification/#platformlocaltxt)
For example, you can try adding this flag back during CI build by creating a platform.local.txt file before building the sketches (https://arduino.github.io/arduino-cli/0.21/platform-specification/#platformlocaltxt)
Changed the PR to keep platform.txt intact for CI and modify warning cflags in platform.local.txt only - by adding the file to the project.
I tested it and this works with Arduino IDE that doesn't considers warnings as an error when building the sketch.
@SuGlider as far as i understand the platform.local.txt file is supposed to be local — I.e. this is the file which the developer can create locally to adjust some build settings. Im'n not sure we are allowed to include a platform.local.txt file with the installation. Would you consider keeping your previous change to platform.txt and instead creating a temporary platform.local.txt in CI before the build job starts?
One more request, please also check that a warning in one of the examples still causes CI to fail.
@SuGlider as far as i understand the platform.local.txt file is supposed to be local — I.e. this is the file which the developer can create locally to adjust some build settings. Im'n not sure we are allowed to include a platform.local.txt file with the installation. Would you consider keeping your previous change to platform.txt and instead creating a temporary platform.local.txt in CI before the build job starts?
I'd need to understand how the CI scripts work first.
I'm not familiar with those CI building scripts.
~~Not sure if they read platform.local.txt when building.~~ .. OK. Arduino-Builder does it.
Another way to move forward, would be to change platform.txt to remove Werror=all from "IDE cflags" and then create another CI.cflags.xxxx that would be picked up by CI to build it.
This way we would have separated sets of cflags for each process.
~~Do you see a potential issue if we create separated CI.xxx.yyy setup in platform.txt?~~
Sounds like using platform.local.txt created in CI Building Time may be easier!
One more request, please also check that a warning in one of the examples still causes CI to fail.
I found warnings from libraries/BLE/examples/BLE5_periodic_sync/BLE5_periodic_sync.ino and I2S example.
I'll check all the CI runners log output.
I talked to @Ouss4 and I'll rework it further. Run some CI testing within this PR and so on.
OK. @igrr @Ouss4
It is now working as desired: 1- platform.txt (user IDE) has -Werror=all removed. 2- Reorganized platform.txt to separate clearly Warning flags and allow those to be easily managed (created .w_flags) 3- Created tools/platform.local.txt that is only used (copied) in CI time by install-arduino-core-esp32.sh script 4- All warning setup/cflags can now be changed in tools/platform.local.txt - it is only used by CI building process 5- I have removed all the waiving warnings flags (-Wnoxxxxx) and now we have many errors in CI, but this means it worked fine.
We shall decide which warning flags should stay in CI. Suggestions are welcome.
One note, platform.txt file is generated by arduino-lib-builder: https://github.com/espressif/esp32-arduino-lib-builder/blob/29387059e8a15b7afb526bdfca9bb9f1c7d65b28/tools/copy-libs.sh#L459-L477.
I think we should update the template used in arduino-lib-builder, otherwise the changes in platform.txt will be lost upon the next update.
Edit: maybe what the script is doing is actually editing the previous platform.txt file; in which case it's probably fine to make the changes here.
One note, platform.txt file is generated by arduino-lib-builder: https://github.com/espressif/esp32-arduino-lib-builder/blob/29387059e8a15b7afb526bdfca9bb9f1c7d65b28/tools/copy-libs.sh#L459-L477.
I think we should update the template used in arduino-lib-builder, otherwise the changes in platform.txt will be lost upon the next update.
Edit: maybe what the script is doing is actually editing the previous platform.txt file; in which case it's probably fine to make the changes here.
I'll also check it and make sure if we must change something there as well.
Issue update: This PR needs more investigation and proper testing so it's not visible for 2.0.5 milestone.
BTW, is the plan to then fix all the warnings? Will this be part of this PR?
Yes, we shall fix all the warning, maybe in separated PRs along time, before merging this one.
what is the point of splitting the flags in such way? They are taken from ESP-IDF and filled in platform.txt and overwriting them could cause issues in headers, etc. All you need to do is edit two lines (removing -Werror=all from platform.txt) and create in CI platform.local.txt with -Werror=all for compiler.warning_flags.all, like echo "compiler.warning_flags.all=-Wall -Werror=all -Wextra" > platform.local.txt
what is the point of splitting the flags in such way? They are taken from ESP-IDF and filled in
platform.txtand overwriting them could cause issues in headers, etc. All you need to do is edit two lines (removing-Werror=allfromplatform.txt) and create in CIplatform.local.txtwith-Werror=allforcompiler.warning_flags.all, likeecho "compiler.warning_flags.all=-Wall -Werror=all -Wextra" > platform.local.txt
What is "CI"?
What is "CI"?
Continuous integration (CI) automatically builds, tests, and integrates code changes within a shared repository. https://resources.github.com/ci-cd/
What is keeping us from making progress on this PR? What examples need to be fixed?
@SuGlider closing this for now. we can loo into better implementation on the current platform later