Skip to content

build in parallel and some cleanup#156

Merged
PerMalmberg merged 19 commits into
PerMalmberg:masterfrom
peterus:update_build_scripts
Apr 15, 2021
Merged

build in parallel and some cleanup#156
PerMalmberg merged 19 commits into
PerMalmberg:masterfrom
peterus:update_build_scripts

Conversation

@peterus
Copy link
Copy Markdown
Contributor

@peterus peterus commented Apr 9, 2021

I did some small tasks in this pull request:

Comment thread .github/workflows/ci.yml Outdated
Comment thread CI/build_smooth_host.sh Outdated
@PerMalmberg
Copy link
Copy Markdown
Owner

Should we take the opportunity to also set a specific IDF-version as discussed in #150 in this PR or shall we do that in a separate PR?

I'm thinking that using a .env file with the IDF-version would be the best way. That'd allow for a single place to set the IDF-version from which both docker build and docker-compose can pick it up, both when run locally and and in CI, assuming the file can be placed in a location found by both.

By updating docker.compose.yml, Dockerfile and build-docker-image.sh we could even allow easy switching between versions locally if someone wants to run against another version than what we set as default (v4.2?)

@peterus
Copy link
Copy Markdown
Contributor Author

peterus commented Apr 10, 2021

from an other thread I was reading that you like building against the latest master branch. So I was not considering this solution.
But if you like to build against a dedicated IDF version I can definitely do this with the .env file. I like your idea with the .env file, with this everything is in one place.

@PerMalmberg
Copy link
Copy Markdown
Owner

I do like building against the master branch, but now that Espressif (allegedly) have fixed the PSRAM issues that were only available in master, that need has lessened. And, with the sudden upsurge in users of Smooth, it makes sense to build against an official release of IDF, while still giving the option to build against master by simply changing a file and building the image locally.

peterus and others added 4 commits April 10, 2021 11:05
Updates for new I2C sensor SHT30 (#150)
- load IDF-version from .env file
- set IDF-version in Dockerfile
- use the IDF-version for the new tag
- read new image name from .env file for fast switching
@peterus
Copy link
Copy Markdown
Contributor Author

peterus commented Apr 10, 2021

as you can see now, with the .env file it is very easy to switch versions. also changing the docker image name is now simple.

If it is okay for you I would try to implement some kind of check if there is a newer IDF image available, building the new image and uploading it to your docker hub or github packages if all tests are successful. of course i would need some kind of API key in the github actions environment.

edit: it would be even possible to try/check different IDF-versions on github actions.

@PerMalmberg
Copy link
Copy Markdown
Owner

Very nice.

I'm not sure automatically updating the image is the right way to go as it introduces a moving target in he build chain. However, a step that fails if there is a new release available could be a good thing if it isn't set to be a required success in the branch protection settings.

I like the idea of building against both latest and the latest major branch of IDF.

peterus added 2 commits April 11, 2021 19:46
- build host image local
- update github actions to build different IDF-versions
@peterus
Copy link
Copy Markdown
Contributor Author

peterus commented Apr 11, 2021

I had an idea which I want to present to you:

  • I splited up the image into a host and esp32 image. As the host image is very small and building up very fast (nearly the same time as docker has to download the image), it will be build as it will be needed. with this you do not have to publish a dedicated docker image.
  • with the help of github actions the CI can run against various IDF-versions (I tried 4.0 until 4.3 and the master branch, IDF-version 4.0 and 4.1 are failing because of interface issues (which is expected)).

I like this approach very much as it is very clean and easy to maintain.

@PerMalmberg
Copy link
Copy Markdown
Owner

Oh, that's a really nice solution. Once Ubuntu 20.04 is available we can update the script to that.

use Ubuntu 20.04 for the docker image
@peterus
Copy link
Copy Markdown
Contributor Author

peterus commented Apr 11, 2021

Ubuntu 20.04 is already available in docker :)
I updated the Dockerfile

edit: i just checked Dockerhub: there is even a 20.10 and 21.04 available. I will try the 21.04 in the end.
edit: Ubuntu 21.04 looks like is not stable. In the CI there was a hickup with the unit test, locally there was an issue with apt-get update. So I switched to 20.10 which is running fine.

Copy link
Copy Markdown
Owner

@PerMalmberg PerMalmberg left a comment

Choose a reason for hiding this comment

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

Maybe a note in the readme explaining the build process, how to select an IDF-version locally?

Also, please update the contributors file to list this PR

Comment thread CI/host-docker/Dockerfile Outdated
Comment thread .env Outdated
Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/ci.yml Outdated
@peterus
Copy link
Copy Markdown
Contributor Author

peterus commented Apr 15, 2021

From my side I am now happy with the current changes.
If you still miss something, I would be happy to hear them.

Copy link
Copy Markdown
Owner

@PerMalmberg PerMalmberg left a comment

Choose a reason for hiding this comment

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

Just a few typos to fix before merging.

Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
@peterus
Copy link
Copy Markdown
Contributor Author

peterus commented Apr 15, 2021

Ah stupid mistakes. I fixed them all.

@PerMalmberg PerMalmberg merged commit 5578b8b into PerMalmberg:master Apr 15, 2021
@PerMalmberg
Copy link
Copy Markdown
Owner

Awesome, merged!

@peterus peterus deleted the update_build_scripts branch April 15, 2021 16:06
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.

2 participants