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

Allow usage of $(location ...) in rustc_env as well, to include_str!() generated files. #503

Merged
merged 2 commits into from
Dec 8, 2020

Conversation

dae
Copy link
Contributor

@dae dae commented Nov 24, 2020

The ${pwd} resolution in build_script_env is necessary during the compilation stage as well, as otherwise we can't use things like include_str!() on generated files. Starting with a test that demonstrates the issue; will follow up with a fix.

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

dae commented Nov 24, 2020

For some reason I was labouring under the impression that this would require a change to process_wrapper, but it appears we can just rely on the existing substitution. In any case, the docstring in utils.bzl explains the need for this:

    $(execroot ...) and $(location ...) are prefixed with ${pwd},
    which process_wrapper and build_script_runner will expand at run time
    to the absolute path. This is necessary because include_str!() is relative
    to the currently compiled file, and build scripts run relative to the
    manifest dir, so we can not use execroot-relative paths.

    $(rootpath ...) is unmodified, and is useful for passing in paths via
    rustc_env that are encoded in the binary with env!(), but utilized at
    runtime, such as in tests. The absolute paths are not usable in this case,
    as compilation happens in a separate sandbox folder, so when it comes time
    to read the file at runtime, the path is no longer valid.

@dae dae marked this pull request as ready for review November 24, 2020 06:20
@dae
Copy link
Contributor Author

dae commented Nov 24, 2020

@UebelAndre this also allows us to move the function into utils as you wanted :-)

Copy link
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me! Will give others a little time to chime in before merging

@dae
Copy link
Contributor Author

dae commented Dec 7, 2020

Rebased over master; gentle ping.

@illicitonion illicitonion merged commit 5448421 into bazelbuild:master Dec 8, 2020
@illicitonion
Copy link
Collaborator

Thanks! Sorry for the delay :)

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.

None yet

2 participants