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

fix(ci): vendor deno for builds #3020

Merged
merged 1 commit into from
Aug 24, 2021
Merged

fix(ci): vendor deno for builds #3020

merged 1 commit into from
Aug 24, 2021

Conversation

rwaskiewicz
Copy link
Member

@rwaskiewicz rwaskiewicz commented Aug 24, 2021

Why is CI failing?

Stencil has experimental support for Deno, and as such needs its dependencies during the build process. Similar to running npm ci, we would pull our dependencies from Deno. Something has changed, although I can't pinpoint it well, where we can't pull the deps we need reliably.

What does this PR do?

This PR takes a 'fresh enough' cache of our Deno dependencies, and vendors them under scripts/build/deno-deps. I say 'fresh enough' because it's hard to reliably pull these dependencies locally if one were to bust their cache, so I'm using my 'best known' version of things here.

Commit Message

add deno source files (vendor them) for more reproducible builds. these
files are being committed to the repo to help stabilize the CI process,
which as been failing for 24 hours.

the stencil team still considers deno to be experimental, this commit
does not mean to imply further development on deno support

FAQ

Is vendoring these a bad idea?

I'd argue no. Although running npm install or npm ci with NPM/node and relying on package-lock.json to create reproducible builds, Deno does not take this approach. The suggestion for production builds is to do something very close to what I have here

Why aren't we using deno cache? Wouldn't that be a better alternative?

It probably would be better to use deno cache. I did attempt this by running DENO_DIR=./src/sys/deno/deno-deps deno cache --unstable src/sys/deno/deps.ts, which effectively downloads those dependencies for you. I was able to download those files successfully. BUT! This had two issues:

  1. The files downloaded are slew of JSON files and hash-named TS files, and I can't figure out how to get them to resolve properly (or if any work is really needed to do that, if it's Deno-magic or not).
  2. Stencil assumes that a Deno source file, resolved by an identifier (the importee used in a file) can be transformed by us. So there's a bunch of tweaking I'd have to do to the deno-std-plugin at the very least to transpile things. This short circuits the process for now

What Risk are we Assuming?

That we could break existing Deno users, although I don't think there's conclusive evidence Deno support really works ATM

This PR also assumes that my fresh enough cache is fresh. If someone wants to double check, we could have them copy their scripts/build/deno-cache off to the side, then pull this branch, then replace what I have here. Any diff could/should be discussed. FWIW, I did a diff of two stencil directories I had, and didn't see any diffs

What should follow this PR?

  1. an ADR for this decision
  2. an RFC issue in the Stencil repo about removing Deno

@rwaskiewicz rwaskiewicz reopened this Aug 24, 2021
@rwaskiewicz rwaskiewicz changed the title small test fix(ci): vendor deno for builds Aug 24, 2021
@rwaskiewicz rwaskiewicz marked this pull request as ready for review August 24, 2021 15:48
@rwaskiewicz rwaskiewicz requested a review from a team August 24, 2021 15:48
add deno source files (vendor them) for more reproducible builds. these
files are being committed to the repo to help stabilize the CI process,
which as been failing for 24 hours.

the stencil team still considers deno to be experimental, this commit
does not mean to imply further development on deno support

STENCIL-87: Resolve CI failing due to Deno
Copy link
Contributor

@splitinfinities splitinfinities left a comment

Choose a reason for hiding this comment

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

These changes make sense. Great PR description as well.

@rwaskiewicz rwaskiewicz merged commit 6d8a61d into master Aug 24, 2021
@rwaskiewicz rwaskiewicz deleted the rwaskiewicz/fetch-test branch August 25, 2021 20:40
rwaskiewicz added a commit that referenced this pull request Aug 27, 2021
add deno source files (vendor them) for more reproducible builds. these
files are being committed to the repo to help stabilize the CI process,
which as been failing for 24 hours.

the stencil team still considers deno to be experimental, this commit
does not mean to imply further development on deno support

STENCIL-87: Resolve CI failing due to Deno
rwaskiewicz added a commit that referenced this pull request Sep 16, 2021
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