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

oxc_traverse: cannot publish the build script #3280

Closed
Boshen opened this issue May 14, 2024 · 6 comments
Closed

oxc_traverse: cannot publish the build script #3280

Boshen opened this issue May 14, 2024 · 6 comments
Assignees
Labels
C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior

Comments

@Boshen
Copy link
Member

Boshen commented May 14, 2024

I had to nuke the build script and revert it back

6d63f99#diff-cb940282a3e680adf99d07ea6f52e94c5131d9e68153c8510abcecd6d166ceb9

482dcc0

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

@Boshen Boshen added the C-bug Category - Bug label May 14, 2024
@Boshen Boshen self-assigned this May 14, 2024
@Boshen Boshen added C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior and removed C-bug Category - Bug labels May 14, 2024
@overlookmotel
Copy link
Collaborator

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
Copy link
Collaborator

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?

@Boshen
Copy link
Member Author

Boshen commented May 22, 2024

Sounds good to me, leave this to me.

@Boshen
Copy link
Member Author

Boshen commented Jun 8, 2024

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

include = ["/src"]

@Boshen Boshen closed this as completed Jun 8, 2024
@overlookmotel
Copy link
Collaborator

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).

@Boshen
Copy link
Member Author

Boshen commented Jun 8, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior
Projects
None yet
Development

No branches or pull requests

2 participants