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

Use tag instead of release branch #79

Closed
wants to merge 1 commit into from

Conversation

MabezDev
Copy link
Member

The release branch can be updated causing inconsistencies among different users depending on when they clone the repo. Safer to use release tags.

…ausing inconsistencies among different users
@justahero
Copy link
Contributor

Hello @MabezDev ,

thanks for the PR, the change set looks good to me. I approved the PR in order to run the CI workflow.

Unfortunately the CI build fails. The CI pipeline is in place to compile all libs & exercises in a Docker environment, to ensure the exercises work with the latest version in main. Could you have a look, maybe you have a better idea why the build fails? Do we need to update the Dockerfile accordingly?

@MabezDev
Copy link
Member Author

Looks like CI has been failing for around a month https://github.com/ferrous-systems/espressif-trainings/commits/main? If I'm honest I'm not an expert on containers, @SergioGasquez perhaps you could take a look? Just from a skim of the log, I don't know why its trying to build PIO, it should be using the native feature of esp-idf-sys 🤔.

@SergioGasquez
Copy link
Member

It looks like it's something related to the activation of the environment (due to the crates), some time ago I made this PR: #69 bumping some crates version. The error that is appearing in the CI of #69 is esp-rs/esp-idf-svc#98. I could use that PR to update all the crates version to the latest if you think it would be beneficial.

@justahero
Copy link
Contributor

Hello @MabezDev & @SergioGasquez ,

I tried to debug the setup to fix the current development environment with Docker & to have CI compile all exercises again. My suspicion is the setup of the ESP framework in the Dockerfile when calling git clone sets up a different environment than a few weeks ago. The referenced branch "release/v4.4" is a not fixed commit. Since the last time I set up the environment (in May) the branch "release/v4.4" has received a few hundred commits. Even though the content of the Dockerfile did not change, the underlying environment & dependencies most likely have. Unfortunately that breaks all builds at the moment.

It looks to me the referenced crates in the different Cargo.toml files conflict with the environment now, the build scripts of the esp-* related crates are missing files for example during compilation. Even trying to find an earlier working revision in the release/v4.4 branch was without luck so far. I would like to use a specific revision (or better a tag) rather than the release branch to set up a fixed environment. Hope this helps a bit.

Is there something you can think of that we can try or test to fix this issue?

@SergioGasquez
Copy link
Member

I was able to compile all the projects merging Scott's changes to #69 and changing the Dockerfile to use v4.4.1 instead of release/v4.4. See https://github.com/SergioGasquez/espressif-trainings/tree/test-fix-tag

@justahero
Copy link
Contributor

Hello @SergioGasquez ,

thanks a lot for testing the setup. 👍

I checked out your branch test-fix-tag, used the resulting Docker image to compile all projects, which worked fine without any issues.

Could you either add the missing commits from your branch to this PR here or open a new PR with all the combined adjustments (what you prefer)? Your feature branch contains a few version bumps for the esp-idf-sys, esp-idf-hal & other crates which seem to be required to compile all exercises.

This should fix PRs #69, #79 (this one) in one go & fix the Docker / CI enviornment.

@SergioGasquez
Copy link
Member

Just created #80! As you mentioned, this PR could resolve #69 and #79

@justahero
Copy link
Contributor

Thanks a lot.

Commit is part of #80.

@justahero justahero closed this Jul 19, 2022
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.

3 participants