Skip to content

Removes -Werror=all from cflags #7060

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

Closed
wants to merge 12 commits into from
Closed

Removes -Werror=all from cflags #7060

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ee8839d
Removes -Werror=all from cflags
SuGlider Jul 29, 2022
9e51e99
Merge branch 'master' into cflags--WError
SuGlider Jul 29, 2022
57db582
Merge branch 'espressif:master' into cflags--WError
SuGlider Aug 4, 2022
8df4963
uses platform.local.txt instead
SuGlider Aug 4, 2022
1688753
CI testing
SuGlider Aug 5, 2022
22cb8c7
CI testing - now should pass all CI testing cases
SuGlider Aug 5, 2022
4fbc7f7
adds platform.local.txt only to CI, by using script
SuGlider Aug 5, 2022
7de323a
fixes typos
SuGlider Aug 5, 2022
1cabb07
fixes CI script to copy platform.local.txt
SuGlider Aug 5, 2022
1b5c4d9
fixes CI PIO script to copy platform.local.txt
SuGlider Aug 5, 2022
bda1bc1
Adds platform.local.txt to cache monitoring
SuGlider Aug 5, 2022
0623d38
Added -Wno-error=format-overflow
SuGlider Aug 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/scripts/install-arduino-core-esp32.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ if [ ! -d "$ARDUINO_ESP32_PATH" ]; then
cd esp32
#git submodule update --init --recursive > /dev/null 2>&1

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

Comment on lines +30 to +32
Copy link
Contributor

Choose a reason for hiding this comment

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

Wasn't this inside the if else statement above? It didn't work?
It should be added after line 20 (inside the if).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it didn't work in side the if

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had to move it outside in order to really copy the file

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it was inside the else not the if where it should be. Also there is a cd missing (or the path needs to be updated accordingly by just adding esp32/)

Copy link
Contributor

Choose a reason for hiding this comment

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

You didn't force push, so we it's still here: 1cabb07

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, thanks for clarifying.
The absolute most of Arduino Users will never use tests_build.sh.
Therefore, I'd say that if it happens, the user that does use it, is an expert one, and that person will know what to do.

I still see no problem with the script copying the file to the Docker file system.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's take one step back please for me to understand the issue. What's the problem with the below diff? It doesn't work?

diff --git a/.github/scripts/install-arduino-core-esp32.sh b/.github/scripts/install-arduino-core-esp32.sh
index 8584da5b..02f29c6a 100755
--- a/.github/scripts/install-arduino-core-esp32.sh
+++ b/.github/scripts/install-arduino-core-esp32.sh
@@ -18,6 +18,8 @@ if [ ! -d "$ARDUINO_ESP32_PATH" ]; then
     if [ ! -z "$GITHUB_REPOSITORY" ];  then
         echo "Linking Core..."
         ln -s $GITHUB_WORKSPACE esp32
+        echo "Copying CI restrictive warning settings from tools/platform.local.txt"
+        cp esp32/tools/platform.local.txt .
     else
         echo "Cloning Core Repository..."
         git clone https://github.com/espressif/arduino-esp32.git esp32 > /dev/null 2>&1

I still see no problem with the script copying the file to the Docker file system.

The issues, as I see them, are:

  1. This is a logic for Github but it's mixed with the common path and not with the Github one.
  2. The script copies build altering files to the home directory unbeknownst to the user and also doesn't clean them.

As for who uses tests_build.sh I hope anyone writing drivers for Arduino would. That means tests are written and validated locally.

Copy link
Collaborator Author

@SuGlider SuGlider Aug 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Let's take one step back please for me to understand the issue. What's the problem with the below diff? It doesn't work?

It didn't work when it is executed by Github Action started by "On Push".
I see that when it uses GitHub-hosted runners with Ubuntu Image, which is created fresh by the runner and it has nothing installed. It always Clone https://github.com/espressif/arduino-esp32.git

This is a logic for Github but it's mixed with the common path and not with the Github one.

This is just a Shell Script executed by a Runner that will use some VM with empty Ubuntu/Mac/Windows files system.
Then the script clones Arduino-IDE, ESP32 Arduino Core into it in order to excute the Tasks.
Right after cloning or linking (which I think never happens with Github Actions), it will copy platform.local.txt from esp32/tools to esp32.

Looking into install-arduino-core-esp32.sh I can see other places that use Common Path - I just used it in the right place.

    if [ ! -z "$GITHUB_REPOSITORY" ];  then
        echo "Linking Core..."
        ln -s $GITHUB_WORKSPACE esp32
    else
        echo "Cloning Core Repository..."
        git clone https://github.com/espressif/arduino-esp32.git esp32 > /dev/null 2>&1
    fi

    #echo "Updating Submodules ..."
    cd esp32                                                   <<<<<<<<<<<<<<<<<<<<<<<
    #git submodule update --init --recursive > /dev/null 2>&1

    echo "Copying CI restrictive warning settings from tools/platform.local.txt"
    cp tools/platform.local.txt .
    
    echo "Installing Platform Tools ..."
    cd tools && python get.py    <<<<<<<<<<<<<<<<<<<<<<<<
    cd $script_init_path

    echo "ESP32 Arduino has been installed in '$ARDUINO_ESP32_PATH'"
    echo ""

The script copies build altering files to the home directory unbeknownst to the user and also doesn't clean them.

Please clarify when does it happen. It copies files to the VM File System that only exists in the CI and Github Action Runner Time. Everything is destroyed after the end of VM execution. Nothing goes to the User File System, unless the script is executes directly in a Shell Command Line - which I understand is not the purpose of this script.

Anyway, it works with Ubuntu/Mac/Windows and Arduino-Builder. It doesn't work with PlatformIO.

Copy link
Collaborator Author

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 any regular user will run this script to install Arduino tools or its github... This would be the only possible problem with running it as you seem to describe.

I think that this script will only be executed by Github Actions... never by the user.

Copy link
Contributor

Choose a reason for hiding this comment

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

It didn't work when it is executed by Github Action started by "On Push".

Was this ever tried?

Right after cloning or linking (which I think never happens with Github Actions),

You can check every on-push workflow, since the creation of this workflow, and you can see that it actually links the repo and does not clone it. Cloning in Github Action is done with the Checkout Action.
image

image

echo "Installing Platform Tools ..."
cd tools && python get.py
cd $script_init_path
Expand Down
3 changes: 3 additions & 0 deletions .github/scripts/install-platformio-esp32.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member

Choose a reason for hiding this comment

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

PIO does not use any Arduino configuration file.

cd "$PLATFORMIO_ESP32_PATH" && cp tools/platform.local.txt .

echo "PlatformIO for ESP32 has been installed"
echo ""

Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/push.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,9 @@ jobs:
./tools/dist
~/arduino_ide
key: ${{ runner.os }}-${{ hashFiles('package/package_esp32_index.template.json',
'platform.txt',
'tools/get.py',
'tools/platform.local.txt',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this one required?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

'.github/scripts/install-arduino-ide.sh') }}
- name: Build Sketches
run: bash ./.github/scripts/on-push.sh ${{ matrix.chunk }} 15
Expand Down
Loading