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

run build scripts in the crate folder instead of the workspace #427

Merged
merged 1 commit into from
Oct 16, 2020

Conversation

dae
Copy link
Contributor

@dae dae commented Oct 11, 2020

Various crates expect to be able to access files relative to the
crate root, and we were giving them the parent directory (eg main)
instead. An example crate that fails to compile without this change
is blake3.

@google-cla google-cla bot added the cla: yes label Oct 11, 2020
@dae
Copy link
Contributor Author

dae commented Oct 11, 2020

I'm guessing the example script was written with the assumption that it would be running from the parent folder. Presumably there is other code out there that makes such an assumption as well, so this would be a breaking change. There are lots of crates out there, and it would be unfortunate if any crates expecting crate root need to be vendored and modified to work in bazel. If changing the default is out of the question, perhaps an extra argument could be added to cargo_build_script() instead? Cargo raze could then set it automatically.

@dae
Copy link
Contributor Author

dae commented Oct 12, 2020

#391 may be related

@damienmg
Copy link
Collaborator

After some though, I am fine with doing this directly. Can you fix the related tests?

@damienmg damienmg self-requested a review October 16, 2020 09:34
Various crates expect to be able to access files relative to the
crate root, and we were giving them the parent directory (eg __main__)
instead. An example crate that fails to compile without this change
is blake3.
@dae
Copy link
Contributor Author

dae commented Oct 16, 2020

Sure, test fixed.

@damienmg
Copy link
Collaborator

Thanks!

@damienmg damienmg merged commit 9ac12ad into bazelbuild:master Oct 16, 2020
@UebelAndre
Copy link
Collaborator

UebelAndre commented Oct 16, 2020

@damienmg @dae Hey, this seems to have broken CI. Would one of you guys mind taking a look?

edit: Spoke too soon, the culprit appears to be #425

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants