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

Optimize the build order to use cache better. #31

Merged
merged 1 commit into from
Dec 12, 2021

Conversation

patricklodder
Copy link
Member

@patricklodder patricklodder commented Dec 8, 2021

Partially implements #22

Verify:

  • Put static(ish) repository locations at the top because those do not change often
  • Move shasum pins to be right after version/variant declaration because they change when those change
  • Merge architecture detection into the verification step to save an execution step and remove the build environment portability hack

Final:

  • install python as late as possible, before copying entrypoint to stay more developer friendly for now. May need to be changed from a security perspective.
  • move the entrypoint inclusion as far down the process as we can

@patricklodder patricklodder added the enhancement New feature or request label Dec 8, 2021
@patricklodder
Copy link
Member Author

Draft until we can test this.

@patricklodder patricklodder linked an issue Dec 8, 2021 that may be closed by this pull request
@patricklodder patricklodder marked this pull request as ready for review December 9, 2021 23:38
@patricklodder
Copy link
Member Author

This is ready now. Good test that the CI process works. I'm going to get some PRs in now.

xanimo
xanimo previously approved these changes Dec 10, 2021
Copy link
Member

@xanimo xanimo left a comment

Choose a reason for hiding this comment

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

ACK.

1.14.5/bullseye/Dockerfile Outdated Show resolved Hide resolved
Verify:
- Put static(ish) repository locations at the top because those
  do not change often
- Move shasum pins to be right after version/variant declaration
  because they change when those change
- Merge architecture detection into the verification step to save
  an execution step and remove the build environment portability
  hack

Final:
- install python as late as possible, before copying entrypoint to
  stay more developer friendly for now. May need to be changed
  from a security perspective.
- move the entrypoint inclusion as far down the process as we can
Copy link
Contributor

@AbcSxyZ AbcSxyZ left a comment

Choose a reason for hiding this comment

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

As I understand Docker recommendations, the ideal may be:

# Apt install
# Release variables / repository location / static derivated variables
# Clone gitian builder/keys/dogecoin repo
# Shasum variables generation
# Download executables

They usually use apt before variables related to the application, which may change more easily when you develop/edit the content. I understand it like the variable declaration should come as late as possible for a RUN instruction. It's how I understand the standard.

A lot of different situations, and "optimal cache" may really vary of the use, regarding which variable can change. Those Dockerfiles are really designed for a specific version, a lot of things may not change, difficult to know...

In some ways it looks really nice, but with variable defined perhaps too much at the top.

However you want, I'm fine with everything :) We can add suggested changes to follow some recommendations (as I understand it), or keep a more elegant solution with (potentially) fixed variables at the top.

@patricklodder
Copy link
Member Author

patricklodder commented Dec 11, 2021

So, in the 7.5 years release history of Dogecoin there has never been a gitian rebuild after release publication. So shasums, versions, and so on, are much more statically linked to the version/variant/Dockerfile than with a Dockerfile that compiles from source, and definitely changes less than the debian-packaged python, which has seen 5 CVEs this year so that would have been at least 5 changes to, say, a 1.14.3 image if we'd done it back then.

This problem will resolve itself shortly because hadolint will force us to pin versions and do the blood, sweat & tears work of maintaining the dependencies precisely - unless we suppress it, but I'd rather go with the pinning and do the work like a smarty shibe. We just need to find a good way to alert the repo of a needed change, maybe dependabot if it can help us with this. It's on my list to look into as soon as we get through the current series of proposed Dockerfile changes, including this PR.

That said, we also need to pin gitian builder somehow 😕

Copy link
Contributor

@AbcSxyZ AbcSxyZ left a comment

Choose a reason for hiding this comment

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

I agree, it's really fixed variables we may rarely or never change. But it's because it's variables x)

Go for it, it's pretty nice (for me) too.
ACK.

Copy link
Member

@xanimo xanimo left a comment

Choose a reason for hiding this comment

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

this looks good to me and ofc we'll always have time for improving/optimizing things in the future. for now though this looks and works good for me. ACK.

@xanimo xanimo merged commit f4e0f9e into dogecoin:main Dec 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants