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

build: Move to single-file builds for craft #203

Merged
merged 23 commits into from
Apr 22, 2021
Merged

Conversation

BYK
Copy link
Member

@BYK BYK commented Apr 20, 2021

This PR reduces the build times dramatically (10x) by generating
a minimized single-file build of Craft binary via esbuild. Using
a single-file build makes is more portable so one can simply do
curl <CRAFT-URL> and use craft directly instead of doing an
npm install. This is especially useful for our centralized publish
repo flows and actions.

IMPORTANT: This PR sacrifices the update checker as it uses lazy imports for certain things. We can try to make it work with static builds but I'm not sure if it's a feature that's worth the effort.

This PR reduces the build times dramatically (10x) by generating
a minimized single-file build of Craft binary via `esbuild`. Using
a single-file build makes is more portable so one can simply do
`curl <CRAFT-URL>` and use craft directly instead of doing an
`npm install`. This is especially useful for our centralized publish
repo flows and actions.
@BYK BYK marked this pull request as ready for review April 20, 2021 13:38
Copy link
Contributor

@tonyo tonyo left a comment

Choose a reason for hiding this comment

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

🏄

@chadwhitacre
Copy link
Member

This PR sacrifices the update checker as it uses lazy imports for certain things. We can try to make it work with static builds but I'm not sure if it's a feature that's worth the effort.

@BYK You and I talked about this elsewhere, doesn't feel great to sacrifice functionality for technical reasons. If we must, we must.

@BYK BYK requested a review from tonyo April 21, 2021 18:04
@BYK
Copy link
Member Author

BYK commented Apr 21, 2021

@tonyo I took the path for adding update-notifier back and actually us using is-ci to populate the default value of the --no-input argument. Thoughts?

src/index.ts Outdated Show resolved Hide resolved
src/utils/env.ts Show resolved Hide resolved
tsconfig.build.json Show resolved Hide resolved
src/utils/version.ts Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
src/commands/artifacts.ts Show resolved Hide resolved
@BYK
Copy link
Member Author

BYK commented Apr 22, 2021

@chadwhitacre I walked back from using update-notifier as it adds unnecessary bulk and even if there is a way for the author to make it compatible with static linker, they were dismissed without much thought (yeoman/update-notifier#123 (comment), yeoman/update-notifier#159 (comment)). There is a fork but I don't think this is worth such an effort. If we find this feature, I'm quite sure we can implement a simpler version without all this pain.

@BYK BYK enabled auto-merge (squash) April 22, 2021 18:51
@BYK BYK requested a review from chadwhitacre April 22, 2021 18:51
Copy link
Member

@chadwhitacre chadwhitacre left a comment

Choose a reason for hiding this comment

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

Approving with the following caveats:

  1. It's too bad we have to choose between update-notifier and esbuild. :-/
  2. I don't see that we actually use the Docker image ourselves, not sure why we bother building that.
  3. I'm not sure I trust that we won't hit a bug at some point because of the other warnings we're ignoring.
  4. I don't really understand why we're adding dist/craft to the GitHub artifacts we're uploading, since we will be curling from elsewhere in action-prepare-release (and continuing to craft as an action via GitHub ref in prepare).

@BYK BYK merged commit d34cf65 into master Apr 22, 2021
@BYK BYK deleted the byk/feat/single-file-builds branch April 22, 2021 21:11
@BYK
Copy link
Member Author

BYK commented Apr 23, 2021

It's too bad we have to choose between update-notifier and esbuild. :-/

We can get similar functionality by ourselves or resort back to using update-notifier-webpack if we find it necessary. I'm not willing to depend on a package where the author is dismissive of simple requests to make the package easier to use.

I don't see that we actually use the Docker image ourselves, not sure why we bother building that.

It is being used by the Craft GitHub Action:

craft/action.yml

Lines 17 to 24 in d34cf65

runs:
using: 'docker'
image: 'docker://getsentry/craft'
args:
- ${{ inputs.action }}
- ${{ inputs.version }}
- ${{ inputs.action == 'publish' && (inputs.no_merge || inputs.keep_branch) && inputs.no_merge || '--' }}
- ${{ inputs.action == 'publish' && (inputs.no_merge || inputs.keep_branch) && inputs.keep_branch || '--' }}

This action is being used in the publish repo: https://github.com/getsentry/publish/blob/514489dfce586772287ce3bcc7a01c86f49b1ce1/.github/workflows/publish.yml#L60-L65

I'm not sure I trust that we won't hit a bug at some point because of the other warnings we're ignoring.

I'll revisit our yargs usage where most of the warnings are coming from. I made sure all dynamic require paths are covered with the changes in this patch.

I don't really understand why we're adding dist/craft to the GitHub artifacts we're uploading, since we will be curling from elsewhere in action-prepare-release (and continuing to craft as an action via GitHub ref in prepare).

These will be used when cutting a release to GitHub where we attach all the assets from the build to the release tag.

@chadwhitacre
Copy link
Member

These will be used when cutting a release to GitHub where we attach all the assets from the build to the release tag.

... and presumably this copies the assets over so the 90-day retention period doesn't apply.

@BYK
Copy link
Member Author

BYK commented Apr 23, 2021

... and presumably this copies the assets over so the 90-day retention period doesn't apply.

Yup. As far as I'm aware release assets are stored indefinitely.

@chadwhitacre
Copy link
Member

cutting a release to GitHub where we attach all the assets from the build to the release tag

public async getAssetsForRelease(

public async uploadAsset(

👍

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