Bazel install target

[tiago-lqt]

Let me start by thanking all the hard work supporting this build flow. Overall the experience is much better than I've had previously with cmake, particularly on windows where I'm using it atm.

I'm planning on using the OpenEXR library and headers outside of the bazel build toolchain, in concrete development of bindings for rust, and because of this I need some sort of install target, which afaik is not supported yet.
Having poked a bit at bazel seems like it would be possible to add a pkg_tar rule to export the build libs and headers.

@Vertexwahn slack directed me to raise this issue in the repo and to ask you if this would be the right course to take?
With the hard work already done seems possible to refactor BUILD.bazel to add this rule but I'd first like some confirmation this is the right path.

[Vertexwahn]

@tiago-lqt Feel free to add a pkg_tar target to the Bazel build.

You can easily add rules_pkg be adding bazel_dep(name = "rules_pkg", version = "0.10.1") to MODULE.bazel file. In the BUILD.bazel file you will need something like this:

load("@bazel_tools//tools/build_defs/pkg:pkg.bzl", "pkg_tar")

pkg_tar(
    name = "deploy",
    srcs = [
        ..., # add header here - maybe use glob
        ":OpenEXR",
    ],
    extension = "tar.gz",
)

I have no experience with Rust bindings. Put adding a pkg_tar should not be too difficult. Looking forward for your PR. There is also rules_rust maybe this can be used to add a rust_test target to ensure the rust binding works.

[tiago-lqt]

I've made some progress in setting up a new pkg_zip rule. Using zip for now just for convenience but can adapt to pkg_tar in the future. Should also add that the current effort is focused on exporting only the files required by OpenEXR Core.

Unfortunately I've hit a blocker with dependencies. include/Imath/ImathConfig.h is also required by OpenEXR Core and this file is build from a third party dependency, even if its in the same repo. This causes the following error:

in pkg_files rule //:ImathFiles: target '@@imath~//:ImathConfig' is not visible from target '//:ImathFiles'.
Check the visibility declaration of the former target if you think the dependency is legitimate

Does this means we would have to update and publish a new Imath version that updates bazel\third_party\Imath.BUILD?

[Vertexwahn]

@tiago-lqt
ImathConfig.h is not part of the openexr repo. Some CMake magic fetches Imath if it is not on your system. Similar it is done on the Bazel side:

https://github.com/bazelbuild/bazel-central-registry/blob/main/modules/imath/3.1.11/patches/add_build_file.patch.

src/Imath/ImathConfig.h is a public header of cc_library Imath. The file can be referenced with something like this: @imath//:src/Imath/ImathConfig.h

[tiago-lqt]

You're right. I've misread the build files. And thanks to your suggestion managed to get around the issue, but still believe there is a visibility issue.

For context the following comes from testing in the v3.2.4 tag at a1a00ff. I wrote half this comment and then went to test at head and noticed the files in question were removed. Which I've tracked for a commit after that release in ad9d698.

If you see the patch file the action ImathConfig doesn't have the visibility attribute. And this seems to be causing the visibility issue when using @imath//:src/Imath/ImathConfig.h.
To get around this I've commented out bazel_dep(name = "imath", version = "3.1.10", repo_name = "Imath") in MODULE.bazel. This seems to trigger the use of the local bazel\third_party\openexr_deps.bzl.
Then I updated the action ImathConfig in bazel\third_party\Imath.BUILD with visibility = ["//visibility:public"].
This finally worked and all the files show in the archive.

Of course the files in question are no longer in the repo, but this test might mean the file patch you've linked might have to be updated.

[Vertexwahn]

visibility = ["//visibility:public"] is set ->
https://github.com/bazelbuild/bazel-central-registry/blob/de00f8c4d582b105952c4ca687995e94decedbf8/modules/imath/3.1.11/patches/add_build_file.patch#L78

If you tell me what I should add there I can add it ;)

Maybe something like

exports_files([
    "some_heaer.h"
])

is needed. Not sure about this...

Can you open a PR with your changes (e.g. in Draft Mode)? Then its easier for me to see the same error...

[tiago-lqt]

I've pushed a sample commit to my fork. tiago-lqt@5965704 . Note that the commit is the one raising the error:

in pkg_files rule //:ImathHeaderFiles: target '@@imath~//:ImathConfig' is not visible from target '//:ImathHeaderFiles'.
Check the visibility declaration of the former target if you think the dependency is legitimate

and if I change the path being used to @imath//:src/Imath/ImathConfig.h it still errors with

in pkg_files rule //:ImathHeaderFiles: target '@@imath~//:src/Imath/ImathConfig.h' is not visible from target '//:ImathHeaderFiles'.
Check the visibility declaration of the former target if you think the dependency is legitimate

To clarify, the rule where I've added the public visibility is the ImathConfig, not Imath that you've linked to. https://github.com/bazelbuild/bazel-central-registry/blob/de00f8c4d582b105952c4ca687995e94decedbf8/modules/imath/3.1.11/patches/add_build_file.patch#L10
Referencing Imath only seems to output the compiled library, not the header files.

I'm guessing that adding this snippet around line 27 of that patch file would solve the issue.

    template = "config/ImathConfig.h.in",
    visibility = ["//visibility:public"]
)

[Vertexwahn]

@tiago-lqt

1. Must be imath not Imath

pkg_files(
    name = "ImathHeaderFiles",
    srcs = [
        "@imath//:ImathConfig"
    ],
    prefix = "include/Imath",
)

2. visiblity need to be changed -> PR is in place here -> Patch imath bazelbuild/bazel-central-registry#1953 - once the PR is merged we need to switch to bazel_dep(name = "imath", version = "3.1.11.bcr.1")

[tiago-lqt]

For 1. its likely due to trying different options. Both were erroring out but I'll switch to imath for the next iteration.

PR looks good, thanks for setting it up!

[Vertexwahn]

@tiago-lqt bazelbuild/bazel-central-registry#1953 is merged now.