Skip to content

Makefile buff-ification #3700

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Sep 2, 2022
Merged

Makefile buff-ification #3700

merged 14 commits into from
Sep 2, 2022

Conversation

deansheather
Copy link
Member

@deansheather deansheather commented Aug 26, 2022 •
edited
Loading

Gets rid of sequential go builds in favor of concurrent ones.

New targets

# builds all slim
build-slim bin

# builds all fat
build-fat build-full build

# to generate site/out/coder.tar.zst
build/coder-slim_${version}_checksums.sha1
build/coder-slim_${version}.tar
build/coder-slim_${version}.tar site/out/coder.tar.zst

# "redirects" to the versioned targets below
build/coder_${arch}_${os}
build/coder-slim_${arch}_${os}

# builds a go binary
build/coder(-slim)?_${version}_${os}_${arch}(.exe)?

# archives
build/coder_${version}_${os}_${arch}.(tar.gz|zip)

# packages (linux only)
build/coder_${version}_linux_${arch}.(apk|deb|rpm)

# docker image "redirects" to versioned targets below
build/coder_linux_${arch}.tag build/coder_linux.tag
push/build/coder_linux_${arch}.tag push/build/coder_linux.tag

# docker image builds and pushes
build/coder_${version}_linux_${arch}.tag build/coder_${version}_linux.tag
push/build/coder_${version}_linux_${arch}.tag push/build/coder_${version}_linux.tag

# helm packages and redirections
build/coder_helm.tgz
build/coder_helm_${version}.tgz

Time savings

For all 7 slim binaries: 30s vs 88s (~66% time saving)

For an entire release build (minus Docker image pushing): 68s vs 250s (~72% time saving)

TODO:

  • Tidy up some makefile spam, maybe make some functions to reduce the spaminess and heavy use of addprefix
  • Add documentation to the top of each task
  • Switch CI to use make instead of calling scripts directly
  • Add docker image targets
  • Helm package
  • Delete unused sh files
  • Compare outputs from old builds to new builds to ensure they are the same

@deansheather deansheather changed the title Makefile buffification Makefile buff-ification Aug 26, 2022
@deansheather deansheather requested a review from sreya August 26, 2022 19:33
@deansheather deansheather marked this pull request as ready for review August 28, 2022 19:29
sreya
sreya approved these changes Aug 29, 2022
View reviewed changes
Copy link
Collaborator

@sreya sreya left a comment

Choose a reason for hiding this comment

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

The changes look good although they ultimately make the Makefile less approachable for others to modify. I'm more excited about faster builds though 🚀

@deansheather
Copy link
Member Author

they ultimately make the Makefile less approachable for others to modify

This is unfortunate, and I've tried to make things as easy to modify as possible by making all the knobs configurable at the top of the file with variables. Makefiles are complicated for a repo with as many targets and dependency links as ours sadly.

kylecarbs
kylecarbs approved these changes Aug 30, 2022
View reviewed changes
Copy link
Member

@kylecarbs kylecarbs left a comment

Choose a reason for hiding this comment

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

Just two minor things. Unfortunately, this adds complexity to the Makefile, but it was adding complexity to the bash before, so it seems like an overall positive change to me!

Makefile
Comment on lines +91 to +92
release: $(CODER_FAT_BINARIES) $(CODER_ALL_ARCHIVES) $(CODER_ALL_PACKAGES) $(CODER_ALL_ARCH_IMAGES) build/coder_helm_$(VERSION).tgz
.PHONY: release
Copy link
Member

Choose a reason for hiding this comment

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

Could we use this target in CI?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can potentially in the future after I make the changes to build darwin on linux, but we can't yet.

@deansheather deansheather enabled auto-merge (squash) September 1, 2022 12:20
@deansheather deansheather merged commit e1a4f3a into main Sep 2, 2022
@deansheather deansheather deleted the buff-makefile branch September 2, 2022 12:58
kylecarbs added a commit that referenced this pull request Sep 2, 2022
@kylecarbs
revert: Makefile buff-ification (#3700)
cf323e7 
This caused the following issues:
- Slim binaries weren't being updated.
- The coder.tar.ztd was misplaced.
- There is no coder.sha1 file with proper filenames.

This should be reintroduced in a future change with those fixes.
kylecarbs added a commit that referenced this pull request Sep 2, 2022
@kylecarbs
revert: Makefile buff-ification (#3700) (#3848)
e490bdd 
This caused the following issues:
- Slim binaries weren't being updated.
- The coder.tar.ztd was misplaced.
- There is no coder.sha1 file with proper filenames.

This should be reintroduced in a future change with those fixes.
deansheather added a commit that referenced this pull request Sep 3, 2022
@deansheather
Revert "revert: Makefile buff-ification (#3700) (#3848)"
72dec5d 
This reverts commit e490bdd.
deansheather added a commit that referenced this pull request Sep 6, 2022
@deansheather
chore: parallel builds with Makefile (#3854)
419d701 
* Revert "revert: Makefile buff-ification (#3700) (#3848)"

This reverts commit e490bdd.

* fix: fix slim targets in makefile

* fix: don't clobber slim binaries, make sure they're in the correct location
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sreya sreya sreya approved these changes

@kylecarbs kylecarbs kylecarbs approved these changes

@coadler coadler Awaiting requested review from coadler

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@deansheather @sreya @kylecarbs