dyedgreen / deno-sqlite

Deno SQLite module
https://deno.land/x/sqlite
MIT License
409 stars 36 forks source link

Use GitHub Actions to verify that generated files are up-to-date #95

Closed jeremyBanks closed 3 years ago

jeremyBanks commented 3 years ago

As suggested by @dyedgreen at https://github.com/dyedgreen/deno-sqlite/pull/88#issuecomment-729801045, here is an initial implementation of rebuilding generated files on GitHub actions, instead of trusting binary WASM files that are locally built and committed.

dyedgreen commented 3 years ago

Thanks for looking into this @jeremyBanks 😍

Just had a quick look overt this, and had an idea for what to do regarding finding if the files need to be updated: I believe the build does not take that long (assuming the SQLite amalgamation does not need to be rebuild, which should happen almost never), so I think it could be a good idea to just rebuild every time and check if the resulting binary/ js is changed (and commit any changes)

I'll have a more thorough look through this later, but looking good so far 😃

Edit: Looking at the action logs, even when downloading sqlite and rebuilding it, the whole process only takes around 1min, which is perfectly fine. Not sure if it's okay to hit their CDN very time (we may want to skip rebuilding SQLite still, since it's rebuild only very rarely, and the output is a source file, so it can quite easily be verified with a local rebuild/ git diff). But I think we should definitely rebuild the wrapper ever time!

jeremyBanks commented 3 years ago

@dyedgreen That makes sense! I was originally concerned that there would be non-determinism in the build that would add spurious changes on a rebuild, but that doesn't appear to be the case (to be verified for sure), the output seems to be stable, so the easier approach of always rebuilding should be good.

dyedgreen commented 3 years ago

I was originally concerned that there would be non-determinism in the build

That should not be the case, as long as you use the same compiler version, the build should be stable, and if not something is up 😅

jeremyBanks commented 3 years ago

Now: it rebuilds on every commit, before running tests. If running in response to a push event, the action will push the changes back to the original branch and mark the action as a success. If we're running in response to a pull_request event, I think we don't have the original branch context to push back to, so instead we mark the build as a failure since we can't verify it.

This is a little bit weird if you push changes to a pull request that's already in progress: the push event will run in my repository, push the changes, and be successful, image but the action will also run in the pull request and fail, and the push from Actions won't trigger another Actions run so the updated commit won't have a status: image

I'm not sure there's anything that can really be done about that, except to encourage users to build changes locally/in their branch before sending the PR. Keeping bad binaries from slipping in is probably more important than this inconvenience.

Regarding repeatedly hitting the CDNs: unless it's download a ton of data, I'm not too worried about loading resources from other GitHub URLs. The SQLite package download isn't on GitHub, but it's only ~10MB, so I think we can probably get away with that too.

jeremyBanks commented 3 years ago

(When/if you merge, you should probably squash-merge since I'm cluttering this PR with test commits for itself.)

jeremyBanks commented 3 years ago

Converting back to draft because I think I have caching of the Deno installation and dependencies working, and will try pushing that shortly.

jeremyBanks commented 3 years ago

Never mind, reverted that. Installing those Deno dependencies takes only two seconds without caching, it's not worth adding the complexity.

jeremyBanks commented 3 years ago

I may not have time to get back to this for a while.

dyedgreen commented 3 years ago

No worries, there is no rush. Apologies again for keeping you waiting during Christmas 😅😬

jeremyBanks commented 3 years ago

@dyedgreen Done, but I think it won't run on this pull request itself until it's merged into master, so I can't test that part. I guess we'll see whether it works after updating the JSON1 PR.

jeremyBanks commented 3 years ago

Gah. It looks like it didn't verify that the workflow was valid because it didn't run until after merging. >_<

I think that should have been

file_pattern: build/sqlite.js build/sqlite.wasm

instead

@dyedgreen re https://github.com/dyedgreen/deno-sqlite/actions/runs/496712684

dyedgreen commented 3 years ago

No worries. I noticed GH Actions was complaining, so I made a few commits to get it to run. I really hate tinkering with CI, since it's so painful to test :/

dyedgreen commented 3 years ago

Yes, it seems to run now 🥳