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

Configure GitHub CI #177

Merged
merged 6 commits into from
May 10, 2022
Merged

Configure GitHub CI #177

merged 6 commits into from
May 10, 2022

Conversation

jdoubleu
Copy link
Contributor

@jdoubleu jdoubleu commented May 7, 2022 •
edited
Loading

This PR introduces a workflow for the GitHub CI, which will build all example applications. Since all of them require the epd_driver component, the driver is implicitly built as well.

In order for all examples to build successfully, I needed to update some of them. Most of the time it was just changing a default value in sdkconfig.defaults, to e.g. use the custom provided partition table (partitions.csv).

The CI will run on every push to the master branch or for every PR which targets the master branch.

The motivation and benefit behind building all examples in the CI is to validate changes made still compile with different versions of ESP-IDF. This begs one important question: Which ESP-IDF version do you want to actively support? The latest stable release (i.e. v4.4.1 at time of writing)?

If the CI is not yet running for this PR, head over to my fork, where I ran it multiple times (during development): https://github.com/jdoubleu/epdiy/actions/runs/2286199069

As you can see, all examples fail to build with the latest version of ESP-IDF, as explaine din #156. If you would merge this PR now, it would fail for every push. I suggest to create a branch off of this one, to fix the incompatibilities first, then merge both into master.

TODOs

jdoubleu added 5 commits May 7, 2022 13:15
@jdoubleu
setup GitHub Actions CI
8943f28
@jdoubleu
build all projects across multiple versions
0021d5d
@jdoubleu
move WiFi settings to Kconfig for mpd_status example
b7b4ad7 
Instead of requiring the user to create a wifi_config.h file, this will use the recommended way to add user-specific configurations from ESP-IDF.

At the same time this has the benefit, that the example even compiles when the user did not set the WiFi settings. The settings simply default to empty strings.
@jdoubleu
fix build of arduino projects in CI
650d606 
Create a separate task, which installs the arduino-esp32 component first.

Also set to use the custom partition table in sdkconfig defaults of the weather example. Otherwise the example would not successfully build by default.
@jdoubleu
use custom partition table by default in mpd_status example
1dc89e8
@martinberlin martinberlin self-assigned this May 8, 2022
@martinberlin martinberlin added the enhancement New feature or request label May 8, 2022
@martinberlin
Copy link
Collaborator

This is a very interesting contribution that can help a lot on all future additions. I will catch up as soon as I can hopefully next week.

@jdoubleu jdoubleu mentioned this pull request May 8, 2022
Merged
@martinberlin martinberlin added the CI testing Continuous integration label May 9, 2022
@martinberlin martinberlin requested a review from vroland May 9, 2022 20:22
@martinberlin
Copy link
Collaborator

Hello @jdoubleu
I would like to try this. I was just thinking that it would be cool also to first merge #168 that is kind of a big refactoring of how boards are organized done by Micke Prag.

But still this is independent and with a bit of work it will adapt without problem to that changes, since the way to configure the project is still done by menuconfig. About this and the other PR #180 how do you suggest to proceed?

Just tell me what things need to be tested and in what order and I will try my best to organize it in the next days.

@jdoubleu
Copy link
Contributor Author

jdoubleu commented May 10, 2022 •
edited
Loading

As you've mentioned, the CI will simply build all examples. At least it does idf.py set-target esp32, which should create an sdkconfig from the defaults. If that process didn't change, the CI should still work fine.

Nonetheless, I could create a duplicate branch off of this one and rebase it onto #168 to be certain it still works. It is then up to you what to merge first.

Unfortunately, the CI does not run for this PR, yet. In my fork it does, so I can test it there.
EDIT: You can see the PR in my fork, which is based on #168 here: jdoubleu#2
EDIT 2: You can see that it failed, because of #168 (comment). Maybe we should merge this one first, to see if anything breaks. But first let me address the TODOs, then.

Ignore #180 for now, because there would be some breaking changes in there, that need to be addressed first.

@jdoubleu
Copy link
Contributor Author

I've removed the runs on the latest ESP-IDF version. They should be restored with #180.

As you can see at https://github.com/jdoubleu/epdiy/actions/runs/2299846098, everything builds successfully. I suggest to merge this PR first, then.

martinberlin
martinberlin approved these changes May 10, 2022
View reviewed changes
Copy link
Collaborator

@martinberlin martinberlin left a comment

Choose a reason for hiding this comment

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

Looks good to me

@martinberlin martinberlin merged commit b985f16 into vroland:master May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinberlin martinberlin martinberlin approved these changes

@vroland vroland Awaiting requested review from vroland

Assignees

@martinberlin martinberlin

Labels
CI testing Continuous integration enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jdoubleu @martinberlin