Skip to content

Makefile buff-ification#3700

Merged
deansheather merged 14 commits into
mainfrom
buff-makefile
Sep 2, 2022
Merged

Makefile buff-ification#3700
deansheather merged 14 commits into
mainfrom
buff-makefile

Conversation

@deansheather
Copy link
Copy Markdown
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
Copy Markdown
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 🚀

Comment thread Makefile Outdated
@deansheather
Copy link
Copy Markdown
Member Author

deansheather commented Aug 29, 2022

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
Copy Markdown
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!

Comment thread Makefile Outdated
Comment thread 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
Copy Markdown
Member

@kylecarbs kylecarbs Aug 30, 2022

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
Copy Markdown
Member Author

@deansheather deansheather Aug 31, 2022

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