Support custom compiler options for compiling `.metal` files

[rpcase]

To include a header inside a shader that is in a different folder one has to currently do so relatively, for example

#include "../.../header.h"

instead of the preferred

#include "header_folder_path/header.h"

This of course couples the source to the current directory structure.

@thii writes

"It's not possible with how rules_apple compiles metal files today, since they're built as part of resources so you can't pass custom metal compiler flags (i.e. for your example it'd be possible by adding -I . to the metal command). To support this, we probably need a new rule"

[thii]

My suggestion is, instead of allowing adding .metal files to data/resources (how it works today), we move that MetalCompile action to a new rule metal_library (in which you can pass additional compiler options via copts). The rule artifacts then can be consumed anywhere you like.

[rpcase]

@thii has there been any progress on metal_library?

[keith]

I don't think so, but we'd be happy to review PRs here! I think looking at apple_intent_library as a similar rule would help with the implementation

[aaronsky]

I'm testing out something I wrote for my employer that addresses this very issue, in the pull request above. No warranty on it yet, but it does compile at the very least.

[aaronsky]

@rpcase If this still of concern to you, let me know if this works! Any issues with this new rule can be opened as a new ticket.

[jiawen]

Amazing work! I go on vacation and see that this is all put together with tests!

[rpcase]

Thank you @aaronsky! This is certainly of concern to me, and I will let you know as soon as I get a chance to take this for a spin.

[appthumb]

Thanks for the implementation!

Does the 'includes' option of the apple_metal_library rule work? That doesn't seem to work for me, and I have to provide the header search path through the copts with the "-I". The latter works but I'd hope the includes works as expected.

Looking at the implementation, the includes option isn't actually used, so I suppose this is why it doesn't have any effect? Not familiar with bzl language at all, so I may have missed something.

[aaronsky]

@appthumb You're right, I overlooked this in my implementation. Apologies. That said, considering Apple's stated best practices for Metal, I'm not sure this attr should exist at all. Can I hear a little more about your use-case?

[appthumb]

Thanks for looking into this! Really appreciate it.

I agree that getting rid of the includes attribute seems reasonable, given that we don't need other libraries to include metal source code. The major issue I'm facing is how to construct the header search path for the apple_metal_library target itself. What is really missed is something like deps, which allows me to declare header file dependencies.

I'm trying to build MLX using bazel, and MLX has the C++ header files included by the .metal files. Here is the subdirectories of my repository:

mlx-0.9.0
|--mlx
    |--- backend --- metal 
    |--- (some header files)

So I want to compile .metal files under mlx/backend/metal and those files needs to include many header files like #include "mlx/some_header_file.h". Basically I need to add "mlx-0.9.0" to the header search paths of the metal target.

Here is my BUILD.bazel:

cc_library(
  name = "mlx",
  srcs = glob(["mlx-0.9.0/mlx/**/*.cpp"]),
  hrds = glob(["mlx-0.9.0/mlx/**/*.h"]),
  includes = ["mlx-0.9.0"]
)

apple_metal_library(
  name = "metallib",
  srcs = glob(["mlx-0.9.0/mlx/**/*.metal"]),
  out = "mlx.metallib",
  hdrs = glob(["mlx-0.9.0/mlx/**/*.h"]),
  copts = ["-Imlx-0.9.0"],  # To find the include files
)

Note the copts attribute, where I have to use it to inject the mlx-0.9.0 into the header search paths. Even if I completely get rid of the mlx-0.9.0 directory in my repository (so mlx becomes the top-level directory), I still need to add copts=-I. for the .metal files to compile correctly.

Ideally, we don't need to use copts, but something like deps=["mlx"] in the apple_metal_library target would be nice. Granted, deps isn't completely correct here because it's not link-time dependency, but at least it's more explicit than the copts. Maybe there is a better way?