Docker image improvements (CON-282)

[Diegorro98]

I created this workflow because I needed to create an image to perform an automated building for my project, so now I'm apporting the workflow to this repository.
The dockerfile is based on esp-idf docker images (v4.4.2 by default as it is the supported IDF version , but it can be changed with build args).

Also I created a readme file that contains info about the images and build args that can be used with the dockerfile. I wish I had more info about the docker image which tag is chip and its purpose in order to write it into the docs too, but I left some space for that here:

esp-matter/tools/docker/README.md

Line 12 in b970273

- `chip`:

arm64 build has the problem that takes too long (between 2 and 4 hours) to setup the python environment when bootstrapping Matter and more than 1.5 hours building host tools. Here you can see a workflow that tried to build amd64 and arm64 image but it exceded the maximum execution time. That's why I disabled the arm64 build in the last commit.

You can test an amd64 image pulling from my docker hub: https://hub.docker.com/r/diegorro98/esp-matter/tags

Also fixes #89.

As mentioned at #89 (comment)), Maybe there is a WIP workflow on the internal repository or similar to create an image with the same purpose.

[Diegorro98]

Rebased to include python requirements

[shubhamdp]

Hi @Diegorro98, yes we do have a work in progress on this for quite some time. I had Dockerfile and the workflow ready but we were still thinking about adding it to esp-matter.

As mentioned at #89 (comment)), Maybe there is a WIP workflow on the internal repository or similar to create an image with the same purpose.

But, looks like this is the high time that we add it. So, I have already put up an PR internally for the required changes.

My changes uses chip-build-esp32 docker image and yours uses the IDF base image.

I did not consider the IDF v5.0 case. May be we can merge our WIP changes and then you can rebase and put up the extra's that you have on top of it?

[shubhamdp]

@Diegorro98 Thank you very much for contributing

[Diegorro98]

Hi @shubhamdp, thank you for reviewing!
Okey, merge your WIP changes and then lets see what can I contribute to the docker file 😄
Out of curiosity, what is the reason behind using the chip-build-esp32 docker image?

[shubhamdp]

@Diegorro98 We have merged the changes, please take a look. Dockerfile and Workflow

Reason for using chip-build-esp32 is that it already has all the stuff to setup esp-matter. It has IDF and all prerequisites required for connectedhomeip

[Diegorro98]

@shubhamdp I already added extra stuff like arguments, an entrypoint so it can be used easily with docker run and the doc about them.

About the IDF v5.0, I'm not sure how to implement it, I was thinking to use chip-build as base image and rewrite this Dockerfile from the chip-build-esp32 so we can give arguments and can be changed. What do you think about it?

I will do the changes and push them so the idea can be visualized. If the idea is discarded I will return revert the changes by undoing them and pushing.

[Diegorro98]

I thought about other alternative which would copy from the esp-idf image the required files this way:

ARG ESP_IDF_TAG=v4.4.3
FROM espressif/idf:${ESP_IDF_TAG} as idf

ARG VERSION=latest
FROM connectedhomeip/chip-build:${VERSION}

ENV IDF_PATH=/opt/espressif/esp-idf/
ENV IDF_TOOLS_PATH=/opt/espressif/tools

COPY --from=idf /opt/esp/idf /opt/espressif/esp-idf

# Setup the ESP-IDF
WORKDIR /opt/espressif/esp-idf
RUN set -x \
    && ./install.sh \
    && : # last line

But It would lead to bigger image size (more or less 0.7 GB bigger), more processing time (2 min more, which might be irrelevant) and less control over esp-idf because ESP_IDF_TAG would be the only option vs the options that are pushed right now. The only benefit of this is code simplicity in the Dockerfile.

[shubhamdp]

@Diegorro98 better would be that we use the IDF docker images and install connectedhomeip dependencies. shallow clone the esp-matter and run install.sh

Probably what you did earlier

[shubhamdp]

Since, we are building it for multiple idf versions, we may have to modify the workflow to build it for both idf v5.0 and v4.4.3

We can use matrix for this.

[Diegorro98]

@Diegorro98 better would be that we use the IDF docker images and install connectedhomeip dependencies. shallow clone the esp-matter and run install.sh

Probably what you did earlier

@shubhamdp okey, I'm gonna restore that in a new branch and I will post the link to the branch here so you can compare both solutions. I will also build and push the image result to the my esp-matter docker hub.

Since, we are building it for multiple idf versions, we may have to modify the workflow to build it for both idf v5.0 and v4.4.3

We can use matrix for this.

I can do that, no problem 😄

[Diegorro98]

https://github.com/Diegorro98/esp-matter/tree/feature/docker_image_alternative

This is the branch with what I did earlier with some changes.

Images from the actual changes in this pull request and the mentioned branch, each one with idf v4.4.3 and v5.0, are at my docker hub:
https://hub.docker.com/r/diegorro98/esp-matter/tags

[shubhamdp]

Hi @Diegorro98 I have been working on supporting esp-idf v5.0, project-chip/connectedhomeip#24589. Once that gets merge, we will pull those changes into esp-matter and can take this up to build examples with both idf v4.4 and v5.0

[Diegorro98]

Hi @shubhamdp, I have seen that in 2a7abbb you have changed from . to source to run the .sh files.
Should I update it here too?

[HomeACcessoryKid]

Can I comment that the images built by espressif are all tagged as latest.
It would be good to have a tag with the date, like if today, the tag would be 20230213.
Like that, we remember which image we are working on and more such advantages.
Hopefully this will be reflected in the upcoming images for 4.4.4 and 5.0
Thanks in advance!

[Diegorro98]

Rebased in to include latest changes

[Diegorro98]

@shubhamdp ESP-IDF 5.0/5.1 is already supported in current Matter commit right?
Should this PR be merged now? Or are you waiting for release tags to add them to this PR to close #267? or there is any other reason why this PR hasn't been merged yet?

[shubhamdp]

@Diegorro98 I wanted to review this PR since last two weeks but couldn't do that. There are few changes that will be needed to get this merged, I'll review the PR this week and lets get this merged then.

[Diegorro98]

@shubhamdp awesome! thank you for you answer :)

[Diegorro98]

@shubhamdp Changes done! The image tag format will be esp-matter:latest(_idf(_branchOrTag)(_ref))
I also updated the version from v4.4.3 to v5.0.1, as readme.md in main specifies.

[shubhamdp]

I'll give a try to these images on my fork of esp-matter to build examples as well.

[darkxst]

Also need to pass in full SHA ref for the following command to work. It fails with abbreviated ref's.

git fetch origin --depth=1 --recurse-submodules ea5e0ff298e6257b31d8e0c81435e6d3937f04c7

[Diegorro98]

@darkxst Also true, I really thought it worked because the build passed, but it passed because the checkout reference wasn't used due to the error you mentioned in last review. Thank you a lot for the feedback!

[Diegorro98]

I've found that building docker files at GitHub Actions tends to file because of the space; I've also found that setting the following steps at the workflow frees enough space to be able to create the docker image:

jobs:
  build:
     steps:
        - run: sudo rm -rf /usr/share/dotnet
        - run: sudo rm -rf "$AGENT_TOOLSDIRECTORY"

@shubhamdp, what do you think? Should I add it?

[shubhamdp]

I've found that building docker files at GitHub Actions tends to file because of the space; I've also found that setting the following steps at the workflow frees enough space to be able to create the docker image:

jobs:
  build:
     steps:
        - run: sudo rm -rf /usr/share/dotnet
        - run: sudo rm -rf "$AGENT_TOOLSDIRECTORY"

@shubhamdp, what do you think? Should I add it?

If this works then please add that.

[shubhamdp]

@Diegorro98 Sorry for the delay and thanks for making this effort. I have few minor suggestion and we are good to merge this.

[shubhamdp]

@Diegorro98 Please stash squash the commits as well.

[Diegorro98]

@shubhamdp sorry, what do you mean by "stash the commits"?

[shubhamdp]

@Diegorro98 sorry, I meant squash.

[Diegorro98]

@shubhamdp is that okey?

[shubhamdp]

@Diegorro98 Can you please rebase to latest main branch. I pushed these changes internally but failed.

[Diegorro98]

@shubhamdp rebased!

[dhrishi]

Thank you @Diegorro98 for your contribution and patience ;-) :-D

[Diegorro98]

@dhrishi no problem, glad to help! 😊

BTW, I've seen that the workflow didn't work
For some reason, GITHUB_REF_NAME is not valid at docker build and push action build arguments, it should be replaced to github.ref.
I added this fix in my fork to the main branch, but I forgot to add it to the PR.

Also I forgot to include a comment explaining these two steps I added to the workflow:

I've found that building docker files at GitHub Actions tends to file because of the space; I've also found that setting the following steps at the workflow frees enough space to be able to create the docker image:

jobs:
  build:
     steps:
        - run: sudo rm -rf /usr/share/dotnet
        - run: sudo rm -rf "$AGENT_TOOLSDIRECTORY"

@shubhamdp, what do you think? Should I add it?

Should I create a new PR including the fix and the comment?

[shubhamdp]

@Diegorro98 Thanks for bringing it up. I've already raised a PR to fix this, along with few other relevant changes.

[shubhamdp]

@Diegorro98 I've not added the comment though to my fixes, may be you can do that.

[Diegorro98]

@shubhamdp sure! I'm going to create a new PR with the comment then.