tree-sitter / tree-sitter-typescript

TypeScript grammar for tree-sitter
MIT License
341 stars 104 forks source link

Fix build reproducability and CI check #196

Closed hendrikvanantwerpen closed 2 years ago

hendrikvanantwerpen commented 2 years ago

This PR fixes two issues concerning builds and CI.

  1. A bug in script/check-generated-files that would cause the build to fail, even though files were not actually changed. The fix was to update the index before running the diff command.

  2. ~Add lock files to the repository, so that the CI build is run with exactly the same package versions as a local build. Without this, the CI build will always use the latest allowed version, which my produce different results from the local build. With the lock files in the repository, builds become predictable and problems because of accidental differences between CI and local environments are reduced. Updating packages becomes an explicit action: run npm update and commit the updated lock files.~

hendrikvanantwerpen commented 2 years ago

/cc @mjambon

dcreager commented 2 years ago

the CI build is run with exactly the same package versions

Is it the tree-sitter-javascript dependency in particular that's being brittle? If so, I think it would be better to encode that as a version spec in Cargo.toml / package.json / etc, so that downstream consumers of the grammar adhere to those constraints too. Checking in the lock file will make sure that our CI build only ever uses a particular version of tree-sitter-javascript, but doesn't enforce that our consumers use that version.

dcreager commented 2 years ago

encode that as a version spec

(This, in turn, means that we need to be using meaningful version numbers up in tree-sitter-javascript. If we make backwards-incompatible changes to the grammar, we have to bump the version number such that the TS grammar can describe which versions of the JS grammar it works with.)

hendrikvanantwerpen commented 2 years ago

Let me explain what happened in more detail. I had a local checkout of master on which I ran npm install at some point. After updating master, running a build (with npm run build, which runs tree-sitter generate) generated different code than what was in the repository, and that generated code resulted in all kinds of parsing errors and failing tests. Running npm install again did nothing, but running npm update updated tree-sitter and after that all tests worked fine.

I thought adding the lock files would prevent this by making the builds reproducible, and it probably would, but @dcreager suggests that this is not recommended for libraries. If I should have a different workflow for this instead of fixing the lock files, perhaps it is an idea to document this in the README?

hendrikvanantwerpen commented 2 years ago

I looked at the console output of npm update and it did not actually say which dependency was updated. I assumed it must have been tree-sitter, but it might have been tree-sitter-javascript as well.

hendrikvanantwerpen commented 2 years ago

For now I have taken out the lock file changes, and only kept the fix to the check-generated-files script. A consequence of having the generated files check but not the lock files is that CI will fail if an acceptable tree-sitter update is available, which generates different output, even though nothing in this repository has changed.

mjambon commented 2 years ago

@dcreager what do you think of getting rid of tree-sitter-javascript altogether? --> https://github.com/tree-sitter/tree-sitter-typescript/issues/191

mjambon commented 2 years ago

I approved the PR. I'm not clear on whether this PR works for you or not, @dcreager.

mjambon commented 2 years ago

Merging this since nobody's complaining.

hendrikvanantwerpen commented 2 years ago

Thanks @mjambon!