Add support for Bazel #898

Vertexwahn · 2021-01-13T21:34:41Z

This PR helps users of the Bazel build system to integrate openexr in their builds.

Supporting two build systems (CMake and Bazel) is of course more effort to maintain but helps to spread openexr.
In the worst case the Bazel build does simply not work - in the best case, someone who uses Bazel can benefit from it.

Also, other projects support CMake and Bazel side by side (e.g. Catch2, gtest, etc.)

linux-foundation-easycla · 2021-01-13T21:34:44Z

The committers are authorized under a signed CLA.

✅ Vertexwahn (5982a7f, d1c00f9)

meshula

This all looks correct, I left some comments about possible copy/paste remnants from Seurat, and noted that it's probably a good idea to omit the header generation code.

meshula · 2021-01-14T01:37:38Z

BUILD.bazel

+)
+
+DEFINES = [
+    "DISABLE_GOOGLE_GLOBAL_USING_DECLARATIONS",


Is this intentional? My understanding is that this is set for the preprocessor when building OpenEXR, but that definition isn't part of OpenEXR's code.

meshula · 2021-01-14T01:38:14Z

BUILD.bazel

+    ":windows_x86_64": [
+        "NOGDI",  # Don't pollute with GDI macros in windows.h.
+        "NOMINMAX",  # Don't define min/max macros in windows.h.
+        "OS_WINDOWS=OS_WINDOWS",


Is this specific to the Seurat project?

meshula · 2021-01-14T01:38:32Z

BUILD.bazel

+        "_WINDOWS",
+
+        # Defaults for other headers (along with OS_WINDOWS).
+        "COMPILER_MSVC",


meshula · 2021-01-14T01:39:19Z

BUILD.bazel

+        "_USE_MATH_DEFINES",
+
+        # Allows 'Foo&&' (e.g., move constructors).
+        "COMPILER_HAS_RVALUEREF",


meshula · 2021-01-14T01:40:13Z

BUILD.bazel

+        # Allows 'Foo&&' (e.g., move constructors).
+        "COMPILER_HAS_RVALUEREF",
+
+        # Doublespeak for "don't bloat namespace with incompatible winsock


Maybe revise to "Improve compile times by enforcing inclusion of a minimal subset of windows.h"?

meshula · 2021-01-14T01:47:18Z

BUILD.bazel

+)
+
+cc_binary(
+    name = "dwaLookups",


last year, we checked in the secondary generated files, so this generation step should not be necessary. Omitting is probably better to reduce the maintenance footprint.

meshula · 2021-01-14T01:47:27Z

BUILD.bazel

+)
+
+cc_binary(
+    name = "b44ExpLogTable",


last year, we checked in the secondary generated files, so this generation step should not be necessary. Omitting is probably better to reduce the maintenance footprint.

meshula · 2021-01-14T01:47:35Z

BUILD.bazel

+)
+
+genrule(
+    name = "dwaLookupsAutoGen",


last year, we checked in the secondary generated files, so this generation step should not be necessary. Omitting is probably better to reduce the maintenance footprint.

meshula · 2021-01-14T01:47:43Z

BUILD.bazel

+)
+
+genrule(
+    name = "b44ExpLogTableAutoGen",


last year, we checked in the secondary generated files, so this generation step should not be necessary. Omitting is probably better to reduce the maintenance footprint.

third_party/BUILD.bazel

peterhillman · 2021-01-14T02:30:46Z

BUILD.bazel

+################################################################################
+# Build the IlmImf library
+ilm_imf_path = "OpenEXR/IlmImf/"
+


This assumes the old code arrangement with various libraries in IlmBase, and the source for the OpenEXR library in IlmImf, instead of src/lib/OpenEXR. Would it be better to revise the build config so it works with the current master branch?

cary-ilm · 2021-01-14T23:28:34Z

Thank you for the contribution. We discussed this in the Technical Steering Committee meeting. Support for Bazel is a helpful service to the community, although none of the current OpenEXR maintainers have any experience with Bazel or use it regularly. We only recently settled on CMake as the official build system and retired the gnu/autotools configuration. Besides getting this PR fixed as noted above, we're concerned about the ongoing maintenance and support burden. Whenever we make changes to the build setup in the future (add/move/rename a file, etc), someone will need to change, and test, the Bazel build.

Will someone in the user community who uses Bazel commit to providing support for it going forward?

At the very least, before we agree to take this on we'd like to see this integrated into our CI, so that it's tested.

An alternative that we discussed would be providing these files in the "Contrib" folder as example code that others can start from, but not necessarily something expected to work reliably.

Vertexwahn · 2021-01-17T21:08:09Z

I rewrote most of the PR and did this time not base my work on seurat.

The dependency tree now looks like this:

A added a CI workflow that can make sure that Bazel build does work.

Please review again.

Here is a demo that shows you what is needed to consume OpenEXR as C++ Bazel developer:
https://github.com/Vertexwahn/BazelDemos/tree/master/OpenEXR
Effort needed for developer:

git_repository(
	name = "openexr",
	remote = "https://github.com/Vertexwahn/openexr", # when mereged https://github.com/AcademySoftwareFoundation/openexr
	branch = "master",
)

load("@openexr//:bazel/third_party/openexr.bzl", "openexr_deps")

openexr_deps()

Note: Under the hood (openexr_deps()) will fetch zlib and Imath.

This nice solution will work if the PR is added to the master branch.

If this stuff is moved to "Contrib" folder the above example will not work. In this case, a bit different solution is needed.

I would suggest adding this PR to the master (and making the bazel build check "non-voting" = does not block changes if Bazel build breaks - only works as a type of indicator) - then let's see how this works out for the next few weeks/months. I would assume that developers who want to make use of OpenEXR and Bazel will contribute to this project. If this does not work out the Bazel files can still be moved to the "Contrib" folder.

Vertexwahn · 2021-02-03T21:53:14Z

I am willing to support the Bazel build support of OpenEXR for a half year - after this period I can reconsider doing it for another half year - I have also some semi-automation in place on my side to support this task (details here)

cary-ilm · 2021-02-17T01:59:31Z

We discussed this in the steering committee meeting again and decided to accept the contribution, with @Vertexwahn's offer of support and maintenance, thank you.

The CI builds are failing, for reasons that seem unrelated to your changes here, but the tests seems to have run on some outdated files. Can you merge the master branch into this and let's confirm the CI checks pass? Thanks.

Reimplemented bazelization for OpenEXR from scratch Signed-off-by: Vertexwahn <julian.amann@tum.de>

Signed-off-by: Vertexwahn <julian.amann@tum.de>

…st) to Bazel Signed-off-by: Vertexwahn <julian.amann@tum.de>

Vertexwahn · 2021-02-17T21:26:24Z

I did a rebase - looking forward to getting this merged

cary-ilm · 2021-02-19T00:40:10Z

Thanks!

meshula reviewed Jan 14, 2021

View reviewed changes

peterhillman reviewed Jan 14, 2021

View reviewed changes

Vertexwahn requested review from meshula and peterhillman January 17, 2021 21:33

Vertexwahn added 7 commits February 17, 2021 19:11

Add bazel support

82952a7

Reimplemented bazelization for OpenEXR from scratch Signed-off-by: Vertexwahn <julian.amann@tum.de>

Add workflow to make sure that Bazel build works

0e1b049

Signed-off-by: Vertexwahn <julian.amann@tum.de>

Remove IexNamespace header file form source

c31eb8a

Signed-off-by: Vertexwahn <julian.amann@tum.de>

Update to Bazel 4.0.0

a370ff8

Signed-off-by: Vertexwahn <julian.amann@tum.de>

Bump imath version

776bc64

Signed-off-by: Vertexwahn <julian.amann@tum.de>

Add correct sha for imath

5b84dec

Signed-off-by: Vertexwahn <julian.amann@tum.de>

Bazel support: Bump Imath dependency. Port CMake changes (ImfIDManife…

8c5f9ac

…st) to Bazel Signed-off-by: Vertexwahn <julian.amann@tum.de>

cary-ilm merged commit 0e877b1 into AcademySoftwareFoundation:master Feb 19, 2021

cary-ilm added CI v3.0.0-beta labels Mar 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Bazel #898

Add support for Bazel #898

Vertexwahn commented Jan 13, 2021

linux-foundation-easycla bot commented Jan 13, 2021 •

edited

Loading

meshula left a comment

meshula Jan 14, 2021

Vertexwahn Jan 17, 2021

meshula Jan 14, 2021

Vertexwahn Jan 17, 2021

meshula Jan 14, 2021

Vertexwahn Jan 17, 2021

meshula Jan 14, 2021

Vertexwahn Jan 17, 2021

meshula Jan 14, 2021

meshula Jan 14, 2021

Vertexwahn Jan 17, 2021

meshula Jan 14, 2021

Vertexwahn Jan 17, 2021

meshula Jan 14, 2021

Vertexwahn Jan 17, 2021

meshula Jan 14, 2021

Vertexwahn Jan 17, 2021

peterhillman Jan 14, 2021

Vertexwahn Jan 17, 2021

cary-ilm commented Jan 14, 2021

Vertexwahn commented Jan 17, 2021 •

edited

Loading

Vertexwahn commented Feb 3, 2021

cary-ilm commented Feb 17, 2021

Vertexwahn commented Feb 17, 2021

cary-ilm commented Feb 19, 2021

Add support for Bazel #898

Add support for Bazel #898

Conversation

Vertexwahn commented Jan 13, 2021

linux-foundation-easycla bot commented Jan 13, 2021 • edited Loading

meshula left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cary-ilm commented Jan 14, 2021

Vertexwahn commented Jan 17, 2021 • edited Loading

Vertexwahn commented Feb 3, 2021

cary-ilm commented Feb 17, 2021

Vertexwahn commented Feb 17, 2021

cary-ilm commented Feb 19, 2021

linux-foundation-easycla bot commented Jan 13, 2021 •

edited

Loading

Vertexwahn commented Jan 17, 2021 •

edited

Loading