oxc icon indicating copy to clipboard operation
oxc copied to clipboard

oxc_traverse: cannot publish the build script

Open Boshen opened this issue 9 months ago • 1 comments

I had to nuke the build script and revert it back

https://github.com/oxc-project/oxc/commit/6d63f99500c73d41ca2810ceff0e8ff8480e3285#diff-cb940282a3e680adf99d07ea6f52e94c5131d9e68153c8510abcecd6d166ceb9

https://github.com/oxc-project/oxc/commit/482dcc0aedf6b67800c5b7001a5d8d00854b1eec

This is not urgent, but should be resolved before the next release, probably when transformer milestone 1 is done.

Boshen avatar May 14 '24 16:05 Boshen

Hmm. Not sure how to solve this. I assume problem is that it requires NodeJS be installed (which is one reason I disabled it from running on CI).

Ultimate solution is to re-write it in Rust. But it's useful to have it in JS for time being while Traverse API is undergoing churn - much faster to change things.

Have you got link to the CI run which failed before you renamed the build script? I don't know how the publish workflow works.

overlookmotel avatar May 14 '24 17:05 overlookmotel

Boshen also mentioned elsewhere that the build script is adding ~18 secs to compile times.

I think we should tackle this as follows:

  1. Remove the Rust build script for now.
  2. Add comments to the top of oxc_ast/src/ast/*.rs saying that if change the types in these files, need to run the oxc_traverse NodeJS build script manually.
  3. Add a CI task which runs the NodeJS build script, and checks that the generated files already committed match the output of the build script (i.e. catch if AST type defs have been changed, but the build script has not been run to update oxc_traverse accordingly). If there's a mismatch, make CI fail.

@Boshen Does that sound like a good approach to you?

overlookmotel avatar May 22 '24 10:05 overlookmotel

Sounds good to me, leave this to me.

Boshen avatar May 22 '24 11:05 Boshen

Fixed, we don't need to publish build.rs

https://github.com/oxc-project/oxc/blob/b09092c9ecba74cbaa8fd42293cba5668889c4f9/crates/oxc_traverse/Cargo.toml#L14

Boshen avatar Jun 08 '24 09:06 Boshen

Does that ensure that the build script has been run locally and oxc_traverse has been updated for any changes to AST types? I'm a bit unclear if we can rely on build script running automatically in local dev, because the build script is in one crate (oxc_traverse) but relies on files in another crate (oxc_ast).

overlookmotel avatar Jun 08 '24 10:06 overlookmotel

Our CI pipeline disallows publishing untracked files, and I run cargo check prior to publish, so it won't be able to publish if things are accidentally unmatched. It'll probably won't compile if things aren't synced anyway.

Boshen avatar Jun 08 '24 11:06 Boshen