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

[vello_shaders] Bring back the Bazel build hack #621

Merged
merged 2 commits into from
Jun 28, 2024
Merged

[vello_shaders] Bring back the Bazel build hack #621

merged 2 commits into from
Jun 28, 2024

Conversation

armansito
Copy link
Collaborator

Skia, an external consumer of this crate, uses Bazel rules to compile Rust code. Due to limitations in Bazel's rust support, the CARGO_MANIFEST_DIR environment variable doesn't get set properly for nested crates since Bazel seems to assign the workspace root instead (as the BUILD file needs to be specified relative to the external repository root).

Until we figure out a better way to manage the bazel integration, this PR reintroduces a recently removed hack to work around this by allowing the crate manifest path to be assigned explicitly, using a new environment variable called BAZEL_CRATE_MANIFEST_PATH. This is intended to be the absolute path to vello_shaders/Cargo.toml within the bazel sandbox file system.

Skia, an external consumer of this crate, uses Bazel rules to compile
Rust code. Due to limitations in Bazel's rust support, the
CARGO_MANIFEST_DIR environment variable isn't set properly for
nested crates as Bazel always seems to assign the workspace root
instead.

Until we figure out a better way to manage the bazel integration, this
PR reintroduces a recently-removed hack to work around this by allowing
the crate manifest path to be assigned explicitly, using a new
environment variable called `BAZEL_CRATE_MANIFEST_PATH`. This is
intended to be the absolute path to `vello_shaders/Cargo.toml` within
the bazel sandbox file system.
@armansito
Copy link
Collaborator Author

I think a better solution going forward might be to host this bazel file directly in the vello repo, which may fix the issue with CARGO_MANIFEST_DIR.

The reason Skia requires Bazel rules to build the crates (as opposed to listing them as a third-party cargo dependencies, which Bazel does support) is that I couldn't find a cleaner way to conditionally toggle crate features (particularly msl and wgsl) from the client-side Bazel rules.

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

Some of the documentation in that BUILD file suggests that this hack was needed because the shader directory is outside vello_shaders. That is no longer true since #590.

Does that change the equation here?

Approving anyway, because the actual change is fine.

vello_shaders/src/compile/mod.rs Outdated Show resolved Hide resolved
@armansito
Copy link
Collaborator Author

Some of the documentation in that BUILD file suggests that this hack was needed because the shader directory is outside vello_shaders. That is no longer true since #590.

Does that change the equation here?

That was part of the original reason for this hack. I was hoping that #590 would address it but I ran into some limitations. I'll update that comment in Skia when I roll this PR in.

Approving anyway, because the actual change is fine.

Thanks!

@armansito armansito added this pull request to the merge queue Jun 28, 2024
Merged via the queue into main with commit 3ee3bea Jun 28, 2024
16 checks passed
@armansito armansito deleted the bazel-hack branch June 28, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants