-
Notifications
You must be signed in to change notification settings - Fork 539
feat(rust_common): allow compile actions to declare more outdirs #3737
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
base: main
Are you sure you want to change the base?
Conversation
|
@UebelAndre ping! |
illicitonion
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generally looks reasonable - could you add a small test showing this working? Thanks!
|
@illicitonion done, thanks PTAL |
5b828f6 to
ddd7571
Compare
…s will be created This technique is sometimes needed when a proc_macro derives information from the build, and it needs to be consumed by some other tool sort cursor wrote a new unit test for this PR fmt
ddd7571 to
e410375
Compare
|
Test failures do not look related to my change, FWICT |
illicitonion
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, thanks! A couple of small things :)
alexeagle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL
illicitonion
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks!
|
Ok we're down to a real test failure! |
1216b17 to
2efdf44
Compare
2efdf44 to
5917e2e
Compare
|
@illicitonion I'm trying to fix, but I don't know either Rust or Windows and don't have local repro. I believe the bug is in the test, not the three-line prod change, can I just tag the test to skip on windows? |
|
@illicitonion ping! would like to merge before I give up on contributing... |
|
Sorry for the delay here - it looks like even with the test skipping, CI was flaking, but hopefully looks good now! |
|
Here's the real-world macro where I use this feature, as an example: |
|
I now have to rebase this patch at customer sites to get @blorente fixes, can we merge it please. |
This technique is sometimes needed when a proc_macro derives information from the build, and it needs to be consumed by some other tool.
For example this one generates a JSON file as an intermediate output
https://github.com/napi-rs/napi-rs/blob/main/crates/macro/src/expand/typedef/type_def.rs#L11-L12
and then expects developers to call this via a Node.js binary which wraps rustc and also transforms that file to a TypeScript type definition file.
Under Bazel, this is better modeled as a rust_shared_library that produces the binding file (
.soor.dylibfor example) along with that JSON output, then run the Node.js binary as a separate target.