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

Bazel build: Bump Imath version to current master #949

Merged
merged 1 commit into from
Mar 4, 2021
Merged

Bazel build: Bump Imath version to current master #949

merged 1 commit into from
Mar 4, 2021

Conversation

Vertexwahn
Copy link
Contributor

OpenEXR should use the current Imath master

Signed-off-by: Vertexwahn <julian.amann@tum.de>
@cary-ilm
Copy link
Member

cary-ilm commented Mar 3, 2021

Is there a way to do this symbolically, that doesn't depend on the explicit hash value? Are we expected to update this after every commit to Imath's master branch?

@Vertexwahn
Copy link
Contributor Author

Vertexwahn commented Mar 3, 2021

TL;DR: I can pin it to the master of Imath instead of bumping - but this has drawbacks

Long version:

If you do not have a monorepo you have always the dilemma of how to combining different repositories. A focus of Bazel is to have reproducible builds. Therefore, Bazel is not happy about using master it rather expects a commit hash.

Option 1

In the case we want always to use the newest and greatest master of Imath, we could simply do it this way:

    maybe(
        new_git_repository,
        name = "Imath",
        build_file = "@openexr//:bazel/third_party/Imath.BUILD",
        branch = "master",
        remote = "/~https://github.com/AcademySoftwareFoundation/Imath",
    )

Nevertheless this would trigger a Bazel warning:

DEBUG: Rule 'Imath' indicated that a canonical reproducible form can be obtained by modifying arguments commit = "b1b5c7cb5f104bcf904d0925cca3abf3cc0f403f", shallow_since = "1614791708 -0800" and dropping ["branch"]

This is only a warning to the user to pin the Imath version. It does not block building or anything else. The user can pin in the WORKSPACE file a specific Imath version.
Somewhere in the users WORKSPACE file, before including OpenExr:

# Pin Imath - to get red of the warning of using always newest master
http_archive(
    name = "Imath",
    build_file = "@openexr//:bazel/third_party/Imath.BUILD",
    sha256 = "6943dce2b2e8737d7cf1ae58103195b64c51c6330237ec07c12d829745b4c9c0",
    strip_prefix = "Imath-f21e31a85a4a0b4ddcd980a19e448fd7eca72cce",
    urls = ["/~https://github.com/AcademySoftwareFoundation/Imath/archive/f21e31a85a4a0b4ddcd980a19e448fd7eca72cce.zip"],
)

BTW: This works because we use the keyword maybe which means fetch Imath only when it was not fetched before in a different way.

Advantages:

  • No need to bump Imath version
  • User gets a Bazel warning to pin it (can be seen as an advantage or disadvantage)

Disadvantages:

  • master of Imath could be broken -> OpenEXR also broken (maybe you see this as an advantage)
  • In a few years it will not be clear which Imath version has been used exactly, since we have only the information "master" (think about checking out a one year old OpenEXR for some reason - who knows what Imath version should used with this old OpenEXR state - we do not have this knowledge in the repo -> maybe this is not important for you or users of OpenEXR)
  • A Bazel warning is generated - user has to specify a Imath version to get rid of it
  • If the CMakeList.txt file of Imath changes this is maybe not reflected in the Imath.BUILD file and this would break the Bazel build (I could also add a WORKSPACE and BUILD file to the Imath repo and add a bazel build test there to make sure that this does not happen - Bumping Imath manually gives me here some control to check for such issues)

Option 2

Keep it as it is ;)

Disadvantages:

  • Manual bumping needed (-> I will care about it, even if I do not care about it I would expect that an older version of Imath works some time together with a newer OpenEXR version - only if you do incompatible changes this leads to a problem)

Advantages:

  • User has not to care which OpenEXR master can be combined with which Imath master
  • Checking out an old state of OpenEXR will use then an old version of Imath that was created around the same time
  • No Bazel warning - no need for used to pin version to get rid of warning
  • Overriding Imath by a different version is still possible if user wishes this
  • Change of master CMakeLists.txt in Imath repo does not have an effect -> using a controlled way of bumping the version enables me to check if some change in Imath (especially CMakeLists.txt) has an effect on the build

Option 3

Extension of Option 1:
I could think about introducing the variables

   IMATH_REP = "/~https://github.com/AcademySoftwareFoundation/Imath.git"
   IMATH_TAG = "master" 

    maybe(
        new_git_repository,
        name = "Imath",
        build_file = "@openexr//:bazel/third_party/Imath.BUILD",
        tag = IMATH_TAG,
        remote = IMATH_REP,
    )

Similar to option 1.

Which option do you prefer? I can go for Option 1/3 - but this has also some drawbacks

@Vertexwahn
Copy link
Contributor Author

For the records: When going for Option 1 it would make sense to add in the long run a WORKSPACE and BUILD file to the Imath repo and a corrsponding Bazel build test in the Imath repo. If this is not wished I would stay with the current setting.

@cary-ilm cary-ilm merged commit 1439efb into AcademySoftwareFoundation:master Mar 4, 2021
@cary-ilm
Copy link
Member

cary-ilm commented Mar 4, 2021

#944 adds a similar CMake mechanism of indicating the version of Imath. Since I don't use Bazel, I don't really have an opinion about the best approach, so I'll defer to you and let you update it as you see fit. Once Imath is officially released and has release tags, presumably it would be best to fix the version to one of those tags.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants