Removes -Werror=all from cflags #7060

SuGlider · 2022-07-29T18:42:03Z

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

P-R-O-C-H-Y

I agree with this change. Some errors should be marked as warning instead.

igrr

All the examples should be compile-able without warnings. Currently CI detects this due to this -Werror=all flag. If you remove it, you need to add it back somewhere during CI build to make sure that a new warning results in CI build failure.

igrr · 2022-08-03T09:35:16Z

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)

SuGlider · 2022-08-04T01:41:30Z

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.

PilnyTomas

LGTM

igrr · 2022-08-04T07:30:37Z

@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?

igrr · 2022-08-04T10:19:15Z

One more request, please also check that a warning in one of the examples still causes CI to fail.

SuGlider · 2022-08-04T11:16:15Z

@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!

SuGlider · 2022-08-04T11:22:21Z

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.

SuGlider · 2022-08-04T13:46:08Z

I talked to @Ouss4 and I'll rework it further.
Run some CI testing within this PR and so on.

SuGlider · 2022-08-05T17:03:07Z

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.

Ouss4 · 2022-08-05T17:16:22Z

.github/workflows/push.yml

                                            'tools/get.py',
+                                            'tools/platform.local.txt',                                            


Is this one required?

If this file changes, it means that cflags (any) have changed in CI building processs... really not sure if this is necessary.
Let me know.

The cache includes the toolchains and the Arduino IDE. The toolchains are unrelated to this.
As for the IDE, the question to ask is if platform.local.txt will introduce new options that affect it. I think the answer is no, unless platform.txt does so and we override them with platform.local.txt. So I believe it's redundant.
That said, it doesn't really hurt to keep it, I'm just trying to understand if we really need it. A concern would be if we keep customizing it and invalidating the cache everytime for each PR that does so.

I believe that we will change it for finishing this PR and we will not change it for a long while after that.
There may be a very small chance of need for changing something in the platform.local.txt and not in the platform.txt in the future, but as I said, I think it will be very rare.

platform.txt is mainly intended to the Arduino end user and as base for CI.
tools/platform.local.txt is only intended for CI, not used at all by the Arduino IDE.

igrr · 2022-08-05T18:17:09Z

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.

SuGlider · 2022-08-05T18:49:39Z

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.

related to issue espressif#7024

Ouss4 · 2022-08-10T09:33:44Z

BTW, is the plan to then fix all the warnings? Will this be part of this PR?

VojtechBartoska · 2022-08-10T12:28:34Z

Issue update: This PR needs more investigation and proper testing so it's not visible for 2.0.5 milestone.

SuGlider · 2022-08-10T16:31:28Z

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.

me-no-dev · 2022-08-24T11:01:59Z

.github/scripts/install-platformio-esp32.sh

@@ -48,6 +48,9 @@ else
    git clone --recursive https://github.com/espressif/arduino-esp32.git "$PLATFORMIO_ESP32_PATH" > /dev/null 2>&1
 fi

+echo "Copying CI restrictive warning settings from tools/platform.local.txt"


PIO does not use any Arduino configuration file.

me-no-dev · 2022-08-24T11:04:18Z

platform.txt

+compiler.common.w_flags=-Wno-error=unused-function -Wno-error=unused-variable -Wno-error=deprecated-declarations -Wno-unused-parameter -Wno-sign-compare -Wno-error=unused-but-set-variable
+
+#
+# ESP32S2 Support Start


ESP32, not ESP32S2

me-no-dev · 2022-08-24T11:13:07Z

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

gfvalvo · 2022-12-28T22:55:21Z

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 "CI"?

RoCorbera · 2022-12-28T23:37:40Z

What is "CI"?

Continuous integration (CI) automatically builds, tests, and integrates code changes within a shared repository.
https://resources.github.com/ci-cd/

mrengineer7777 · 2023-04-29T02:23:14Z

What is keeping us from making progress on this PR? What examples need to be fixed?

me-no-dev · 2024-01-31T19:20:43Z

@SuGlider closing this for now. we can loo into better implementation on the current platform later

Removes -Werror=all from cflags

ee8839d

SuGlider added this to the 2.0.5 milestone Jul 29, 2022

SuGlider requested review from PilnyTomas and P-R-O-C-H-Y July 29, 2022 18:42

SuGlider self-assigned this Jul 29, 2022

Merge branch 'master' into cflags--WError

9e51e99

SuGlider mentioned this pull request Aug 2, 2022

unused but set variables #7074

Closed

1 task

P-R-O-C-H-Y approved these changes Aug 3, 2022

View reviewed changes

igrr requested changes Aug 3, 2022

View reviewed changes

SuGlider added 2 commits August 3, 2022 22:16

Merge branch 'espressif:master' into cflags--WError

57db582

uses platform.local.txt instead

8df4963

PilnyTomas approved these changes Aug 4, 2022

View reviewed changes

SuGlider marked this pull request as draft August 4, 2022 13:44

SuGlider added 3 commits August 5, 2022 10:30

CI testing

1688753

CI testing - now should pass all CI testing cases

22cb8c7

adds platform.local.txt only to CI, by using script

4fbc7f7

Ouss4 mentioned this pull request Aug 5, 2022

Increase warning level in CI builds. #7090

Closed

fixes typos

7de323a

SuGlider requested a review from Ouss4 August 5, 2022 16:12

fixes CI script to copy platform.local.txt

1cabb07

SuGlider marked this pull request as ready for review August 5, 2022 17:03

fixes CI PIO script to copy platform.local.txt

1b5c4d9

Ouss4 reviewed Aug 5, 2022

View reviewed changes

Added -Wno-error=format-overflow

0623d38

related to issue espressif#7024

VojtechBartoska added the Status: In Progress ⚠️ Issue is in progress label Aug 10, 2022

VojtechBartoska removed this from the 2.0.5 milestone Aug 10, 2022

SuGlider marked this pull request as draft August 11, 2022 01:35

me-no-dev reviewed Aug 24, 2022

View reviewed changes

VojtechBartoska added this to the 2.0.6 milestone Sep 21, 2022

VojtechBartoska added Status: To be implemented Selected for Development and removed Status: In Progress ⚠️ Issue is in progress labels Sep 21, 2022

mrengineer7777 mentioned this pull request Oct 12, 2022

sprintf statement only gives compile error if compiled with compiler warning level "All" and "More" (-Wall -Werror=all) #7024

Closed

VojtechBartoska added the Status: In Progress ⚠️ Issue is in progress label Dec 20, 2022

SuGlider mentioned this pull request Dec 21, 2022

Removes a few Warnings and fixes Math rand() to work like Arduino mainstream #7613

Merged

SuGlider modified the milestones: 2.0.6, 3.0.0, 2.0.7 Dec 21, 2022

VojtechBartoska removed this from the 2.0.7 milestone Feb 13, 2023

mrengineer7777 mentioned this pull request May 3, 2023

Fix CameraWebserver.ino CI Warning #8149

Merged

me-no-dev closed this Jan 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removes -Werror=all from cflags #7060

Removes -Werror=all from cflags #7060

SuGlider commented Jul 29, 2022

P-R-O-C-H-Y left a comment

igrr left a comment

igrr commented Aug 3, 2022

SuGlider commented Aug 4, 2022

PilnyTomas left a comment

igrr commented Aug 4, 2022

igrr commented Aug 4, 2022

SuGlider commented Aug 4, 2022 •

edited

Loading

SuGlider commented Aug 4, 2022

SuGlider commented Aug 4, 2022

SuGlider commented Aug 5, 2022 •

edited

Loading

Ouss4 Aug 5, 2022

SuGlider Aug 5, 2022

Ouss4 Aug 5, 2022

SuGlider Aug 5, 2022

SuGlider Aug 5, 2022

igrr commented Aug 5, 2022 •

edited

Loading

SuGlider commented Aug 5, 2022 •

edited

Loading

Ouss4 commented Aug 10, 2022

VojtechBartoska commented Aug 10, 2022

SuGlider commented Aug 10, 2022

me-no-dev Aug 24, 2022

me-no-dev Aug 24, 2022

me-no-dev commented Aug 24, 2022

gfvalvo commented Dec 28, 2022 •

edited

Loading

RoCorbera commented Dec 28, 2022

mrengineer7777 commented Apr 29, 2023

me-no-dev commented Jan 31, 2024

Removes -Werror=all from cflags #7060

Removes -Werror=all from cflags #7060

Conversation

SuGlider commented Jul 29, 2022

Description of Change

Tests scenarios

Related links

P-R-O-C-H-Y left a comment

Choose a reason for hiding this comment

igrr left a comment

Choose a reason for hiding this comment

igrr commented Aug 3, 2022

SuGlider commented Aug 4, 2022

PilnyTomas left a comment

Choose a reason for hiding this comment

igrr commented Aug 4, 2022

igrr commented Aug 4, 2022

SuGlider commented Aug 4, 2022 • edited Loading

SuGlider commented Aug 4, 2022

SuGlider commented Aug 4, 2022

SuGlider commented Aug 5, 2022 • edited Loading

Ouss4 Aug 5, 2022

Choose a reason for hiding this comment

SuGlider Aug 5, 2022

Choose a reason for hiding this comment

Ouss4 Aug 5, 2022

Choose a reason for hiding this comment

SuGlider Aug 5, 2022

Choose a reason for hiding this comment

SuGlider Aug 5, 2022

Choose a reason for hiding this comment

igrr commented Aug 5, 2022 • edited Loading

SuGlider commented Aug 5, 2022 • edited Loading

Ouss4 commented Aug 10, 2022

VojtechBartoska commented Aug 10, 2022

SuGlider commented Aug 10, 2022

me-no-dev Aug 24, 2022

Choose a reason for hiding this comment

me-no-dev Aug 24, 2022

Choose a reason for hiding this comment

me-no-dev commented Aug 24, 2022

gfvalvo commented Dec 28, 2022 • edited Loading

RoCorbera commented Dec 28, 2022

mrengineer7777 commented Apr 29, 2023

me-no-dev commented Jan 31, 2024

SuGlider commented Aug 4, 2022 •

edited

Loading

SuGlider commented Aug 5, 2022 •

edited

Loading

igrr commented Aug 5, 2022 •

edited

Loading

SuGlider commented Aug 5, 2022 •

edited

Loading

gfvalvo commented Dec 28, 2022 •

edited

Loading