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

feat: update the Dockerfile with emulator dependencies #435

Merged
merged 3 commits into from
Jun 13, 2024

Conversation

renan061
Copy link
Contributor

@renan061 renan061 commented Jun 3, 2024

No description provided.

@renan061 renan061 force-pushed the feature/dockerfile branch from e2a335e to 7ec5c8d Compare June 3, 2024 18:10
@renan061 renan061 self-assigned this Jun 3, 2024
@renan061 renan061 added this to the 2.0.0 milestone Jun 3, 2024
@renan061 renan061 marked this pull request as ready for review June 3, 2024 18:11
@renan061 renan061 changed the base branch from main to next/2.0 June 3, 2024 18:11
@renan061 renan061 force-pushed the feature/dockerfile branch 4 times, most recently from 45adcb7 to fb685af Compare June 3, 2024 18:18
@marcelstanley marcelstanley requested a review from torives June 4, 2024 19:55
Copy link
Collaborator

@marcelstanley marcelstanley left a comment

Choose a reason for hiding this comment

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

I'm assuming it would be preferrable to have a single commit for most of the changes related to the emulator and the CI. In that case, it would be helpful it the commit contained a more thorough explanation about their purpose as the commit message does not convey much about it.

Also, in order to make the changes clearer in the diff, I'd suggest either creating a separate commit for the formatting changes or reverting them altogether. This would help a lot with reviewing Dockerfile and build/docker-bake.hcl (it's a bit hard to identify the upgrades being made to the versions)

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
build/Dockerfile Outdated Show resolved Hide resolved
build/Dockerfile Show resolved Hide resolved
build/docker-bake.platforms.hcl Outdated Show resolved Hide resolved
build/Dockerfile Show resolved Hide resolved
build/Dockerfile Show resolved Hide resolved
build/Dockerfile Show resolved Hide resolved
build/Dockerfile Outdated Show resolved Hide resolved
pkg/addresses/addresses.go Show resolved Hide resolved
@renan061 renan061 force-pushed the feature/dockerfile branch 3 times, most recently from 229a17c to 78a2acb Compare June 6, 2024 17:45
@renan061
Copy link
Contributor Author

renan061 commented Jun 6, 2024

I'm assuming it would be preferable to have a single commit for most of the changes related to the emulator and the CI. In that case, it would be helpful it the commit contained a more thorough explanation about their purpose as the commit message does not convey much about it.

Also, in order to make the changes clearer in the diff, I'd suggest either creating a separate commit for the formatting changes or reverting them altogether. This would help a lot with reviewing Dockerfile and build/docker-bake.hcl (it's a bit hard to identify the upgrades being made to the versions)

I've update the commit message with more information.

Separating the refactor in another commit would further delay this commit.
I think we can get together in a call for me to explain stuff.

Copy link
Contributor

@torives torives left a comment

Choose a reason for hiding this comment

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

Let's just reinstate the linux/arm64 platform now that we know that xgenext2fs has a build for it. Other than that, it looks good to me 👌🏽

@marcelstanley
Copy link
Collaborator

Now that we removed the documentation related to bake targets from the Dockerfile, wouldn't it make sense to add them somewhere else, @renan061 @torives:

@marcelstanley
Copy link
Collaborator

Let's just reinstate the linux/arm64 platform now that we know that xgenext2fs has a build for it. Other than that, it looks good to me 👌🏽

Please close #439 when this is done, @renan061

@renan061 renan061 linked an issue Jun 13, 2024 that may be closed by this pull request
@renan061 renan061 removed request for fmoura and vfusco June 13, 2024 00:44
@renan061 renan061 force-pushed the feature/dockerfile branch from 78a2acb to 12f069d Compare June 13, 2024 00:45
@renan061
Copy link
Contributor Author

renan061 commented Jun 13, 2024

Fixed linux/arm64. Now working!

Regarding the documentation for targets, I edited each stage to make sure the same text is still there.
What changes is that we are not identifying which stages are targets in the Dockerfile anymore.

Because docker-bake.hcl is very explicit about which stages are targets,
I don't think we need to document it elsewhere.

@renan061 renan061 force-pushed the feature/dockerfile branch from 12f069d to 5b38dac Compare June 13, 2024 01:12
@marcelstanley marcelstanley self-requested a review June 13, 2024 15:06
renan061 added 3 commits June 13, 2024 14:05
- Adds linux.bin and rootfs.ext2 download to the emulator stage.
- Adds a emulator-devel stage that install libcmt and xgenext2fs.
- Configures the CI image to use CGO.
- Refactors the Dockerfile to improve readability.
@renan061 renan061 force-pushed the feature/dockerfile branch from 5b38dac to e1b1e24 Compare June 13, 2024 17:05
@renan061 renan061 merged commit 0e3a844 into next/2.0 Jun 13, 2024
6 checks passed
@renan061 renan061 deleted the feature/dockerfile branch June 13, 2024 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Restoring the linux/arm64 platform in the CI
3 participants