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.

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

Add ESP32 and STM32 CI tests to GitHub actions #2629

Merged
merged 3 commits into from Oct 6, 2021

Conversation

amirgon
Copy link
Contributor

@amirgon amirgon commented Oct 3, 2021

Build ESP32 and STM32 Micropython ports with LVGL.

@embeddedt - Any idea why STM32 build fails here?

@embeddedt
Copy link
Member

The only potential issue I see is that you didn't run make -C ports/stm32 submodules before running the real make command, and since you're using -j, I see a potential for it to start scanning QSTRs, etc. before the submodule is actually checked out.

Aside from that, in my opinion, it'd be better to use a matrix and have them each run as separate actions, instead of building them in the same action, like what is done here. If you want I can reorganize it like that within the next couple days.

@amirgon
Copy link
Contributor Author

amirgon commented Oct 4, 2021

If you want I can reorganize it like that within the next couple days.

Sure, thanks!

@embeddedt
Copy link
Member

Okay, this is not as clean as I'd like, but I can't do much better given the inconsistency among the ports. STM32 needs a BOARD argument, Unix needs a VARIANT argument, and ESP32 doesn't use make directly.

At the very least, they are in independent workflows now, so they don't affect each other's build environment and they get listed separately in the checks list. The file also seems a bit easier to follow now, at least for me.

I somehow seem to have messed up the ESP32 build while making these changes but the error doesn't seem related.

error
-- IDF_TARGET not set, using default target: esp32
-- The C compiler identification is GNU 8.4.0
-- The CXX compiler identification is GNU 8.4.0
-- The ASM compiler identification is GNU
-- Found assembler: /home/runner/.espressif/tools/xtensa-esp32-elf/esp-2020r3-8.4.0/xtensa-esp32-elf/bin/xtensa-esp32-elf-gcc
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /home/runner/.espressif/tools/xtensa-esp32-elf/esp-2020r3-8.4.0/xtensa-esp32-elf/bin/xtensa-esp32-elf-gcc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /home/runner/.espressif/tools/xtensa-esp32-elf/esp-2020r3-8.4.0/xtensa-esp32-elf/bin/xtensa-esp32-elf-g++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Building ESP-IDF components for target esp32
CMake Error at /home/runner/work/lvgl/lvgl/esp-idf/tools/cmake/component.cmake:224 (message):
  CMake Error at
  /home/runner/work/lvgl/lvgl/lib/lv_bindings/lvgl/CMakeLists.txt:1
  (project):

    project command is not scriptable

  Call Stack (most recent call first):

    /home/runner/work/lvgl/lvgl/esp-idf/tools/cmake/scripts/component_get_requirements.cmake:97 (include)
    /home/runner/work/lvgl/lvgl/esp-idf/tools/cmake/scripts/component_get_requirements.cmake:115 (__component_get_requirements)

-- Configuring incomplete, errors occurred!
See also "/home/runner/work/lvgl/lvgl/ports/esp32/build-GENERIC/CMakeFiles/CMakeOutput.log".
  

  

Call Stack (most recent call first):
  /home/runner/work/lvgl/lvgl/esp-idf/tools/cmake/build.cmake:414 (__component_get_requirements)
  /home/runner/work/lvgl/lvgl/esp-idf/tools/cmake/project.cmake:396 (idf_build_process)
  CMakeLists.txt:48 (project)


cmake failed with exit code 1
Executing action: all (aliases: build)
Running cmake in directory /home/runner/work/lvgl/lvgl/ports/esp32/build-GENERIC
Executing "cmake -G 'Unix Makefiles' -DPYTHON_DEPS_CHECKED=1 -DESP_PLATFORM=1 -DMICROPY_BOARD=GENERIC -DCCACHE_ENABLE=0 /home/runner/work/lvgl/lvgl/ports/esp32"...
make: *** [Makefile:35: all] Error 2
make: Leaving directory '/home/runner/work/lvgl/lvgl/ports/esp32'
Error: Process completed with exit code 2.

One concern I did have with ESP32 in partiular is that we check out the LVGL commit hash from the PR before make submodules is run. I'm pretty sure that means that if it tries to run a submodule update on lv_bindings, it will end up resetting LVGL back to the commit hash from lv_micropython.

However, I don't see an easy way of solving that, as upstream put the make submodules command inside the build step in ci.sh.

This isn't an issue on Unix and STM32 as we run that step inside our own workflow.

@amirgon
Copy link
Contributor Author

amirgon commented Oct 4, 2021

One concern I did have with ESP32 in partiular is that we check out the LVGL commit hash from the PR before make submodules is run.

You are correct, we should first call make submodules before checking out the correct LVGL commit.

I somehow seem to have messed up the ESP32 build while making these changes but the error doesn't seem related.

Maybe related to this recent cmake commit on LVGL? I suspected it won't work on esp32: #2621

@rzr
Copy link
Contributor

rzr commented Oct 5, 2021

Maybe rebased on #2636 to pass ESP32

Let's check with:

#2639

@embeddedt
Copy link
Member

Okay, the build works now with @rzr's change. Thank you!

we should first call make submodules before checking out the correct LVGL commit.

Should we just copy the commands from ci_esp32_build into the GitHub action so we can tweak them ourselves, then? I think it's the only solution - either that or we change ci.sh in lv_micropython, which doesn't make sense when the issue is in a downstream repository.

@rzr
Copy link
Contributor

rzr commented Oct 5, 2021

great then I'll revert the if(install) part in:

#2636

@embeddedt
Copy link
Member

Actually, now that I think about it, I guess the issue regarding submodules I mentioned must not be a problem at the moment, because otherwise the build behavior wouldn't have changed when I rebased the PR. So let's leave that for now.


The only other complaint I have is that the ESP32 check is really slow compared to the others. In total, Unix takes 3.5 minutes and STM32 takes 4. ESP32 takes just under 10. That's a very long time to wait for feedback on a change.

I split the steps for measurement purposes and it appears that setting up ESP-IDF takes 2 minutes, and actually building the project takes another 6.

Is there an existing option for building a more stripped down version of the port? I saw it compiling a lot of unnecessary components like nimble and mbedcrypto. We don't really need these here since we're just verifying that LVGL still compiles in that environment.

@amirgon
Copy link
Contributor Author

amirgon commented Oct 5, 2021

Is there an existing option for building a more stripped down version of the port? I saw it compiling a lot of unnecessary components like nimble and mbedcrypto. We don't really need these here since we're just verifying that LVGL still compiles in that environment.

This may be possible by configuring the sdkconfig macros. This is usually done by defining a new board that overrides them.
But the problem is that Micropython defines esp32-specific modules with esp32 functionality based on these components (for example, nimble (BLE) functionality) so we would need to disable each of these modules otherwise they won't compile.
It may become harder to maintain it in the future when newer Micropython versions add new esp-specific functionality because if it's disabled on our board the new functionality would break the build.

So my preference is to keep it as close to as possible to upstream Micropython even if the build time is longer.

Actually, now that I think about it, I guess the issue regarding submodules I mentioned must not be a problem at the moment, because otherwise the build behavior wouldn't have changed when I rebased the PR. So let's leave that for now.

make submodules is defined like this:

GIT_SUBMODULES += lib/lv_bindings
...
submodules:
	git submodule update --init $(addprefix ../../,$(GIT_SUBMODULES))

So it would update lv_bindings but not recursively. Actually it's a bug - if we ever fix that (add all lv_bindings submodules to GIT_SUBMODULES, LVGL among them) then this test would stop working.

So we should probably either completely remove lib/lv_bindings from GIT_SUBMODULES, or fix it in this CI test.
But I think that copying the commands from ci_esp32_build to here is a bit ugly. And would be harder to maintain when a newer Micropython version updates ci_esp32_build.

@embeddedt
Copy link
Member

How about we just condition the GIT_SUBMODULES += lib/lv_bindings on an environment variable like IS_LVGL_CI not being set?

@embeddedt embeddedt marked this pull request as ready for review October 6, 2021 16:02
@embeddedt
Copy link
Member

Okay, I'm going to go ahead and merge this in its current state, since it does work for now even though it's relying on a bug, and I'd like it in master to confirm that #2636 doesn't break anything else.

We can fix the submodule issue after this is merged.

@embeddedt embeddedt merged commit a625dc2 into lvgl:master Oct 6, 2021
amirgon added a commit to lvgl/lv_micropython that referenced this pull request Oct 6, 2021
The 'make submodules' rule does not work recursively, so adding only lib/lv_bindings is not enough, and adding all its submodules manually is harder to maintain.

In addition, this doesn't work well with LVGL CI, see lvgl/lvgl#2629 (comment)
@amirgon
Copy link
Contributor Author

amirgon commented Oct 6, 2021

We can fix the submodule issue after this is merged.

Sure.
I removed lib/lv_bindings from GIT_SUBMODULES, that's the simplest solution for now.

@embeddedt
Copy link
Member

Does it need to be removed from py/py.mk as well, after we make sure all the GitHub actions scattered around the repositories do actually clone lv_bindings separately?

@amirgon
Copy link
Contributor Author

amirgon commented Oct 17, 2021

Does it need to be removed from py/py.mk as well,

Right.
I think it should be safe to remove it, it seems that all our CI under GitHub workflows contain something like:

 - name: Initialize lv_bindings submodule
      run: git submodule update --init --recursive lib/lv_bindings

Done in lvgl/lv_micropython#46

amirgon added a commit to lvgl/lv_micropython that referenced this pull request Oct 17, 2021
amirgon added a commit to lvgl/lv_micropython that referenced this pull request Oct 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants