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

[workspace] Add vtk_internal build from source #20068

Merged

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Aug 25, 2023

Towards #19945 and #16502.


This change is Reviewable

@jwnimmer-tri jwnimmer-tri added release notes: fix This pull request contains fixes (no new features) priority: medium labels Aug 25, 2023
@jwnimmer-tri
Copy link
Collaborator Author

@drake-jenkins-bot mac-x86-monterey-clang-bazel-experimental-release please

@jwnimmer-tri
Copy link
Collaborator Author

+@rpoyner-tri for feature review, please.

@jwnimmer-tri jwnimmer-tri marked this pull request as ready for review August 25, 2023 22:42
Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

:lgtm: I haven't thought about TGA files in literally decades.

Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: 7 unresolved discussions, needs at least two assigned reviewers


tools/workspace/vtk_internal/repository.bzl line 47 at r1 (raw file):

        key, values = tokens[0], tokens[1:]
        if key.endswith("S"):
            # Plural key.

This test happens to work with the keys documented, but the overall effect is somewhat misleading.

The distinction in number is nothing to do with the incidental grammar status of the keys; its rather whether the key takes one or multiple values.

Perhaps instead:

multivalued_keys = ["DEPENDS", "PRIVATE_DEPENDS"]  # maybe others later?
if key in multivalued_keys:
    # Multi-valued key.
   # ... blah
else:
    # Single-valued key.

Or, if you are confident that no = characters will ever be embedded in values, just branch on the length of the token list?


tools/workspace/vtk_internal/repository.bzl line 65 at r1 (raw file):

    an loadable `modules.bzl` file in the root of the repository.

    This is necessary because BUILD files can't parse external metdata as part

typo

Suggestion:

metadata

tools/workspace/vtk_internal/rules.bzl line 59 at r1 (raw file):

            module_deps_private.remove(dep_name)

    # Generate the module header, ala CMake GenerateExportHeader.

typo

Suggestion:

a la

tools/workspace/vtk_internal/rules.bzl line 75 at r1 (raw file):

            ]).format(scream = scream),
        )
        hdrs_extra = hdrs_extra + [module_h]

nit does not hdrs_extra.append(module_h) work in Starlark?

At least one other instance below.


tools/workspace/vtk_internal/rules.bzl line 92 at r1 (raw file):

    ] + hdrs_glob_exclude)
    gen_hdrs = [
        "gen/" + x[:-3]

This slice is a masterpiece of obfuscation. What exactly is the goal here?


tools/workspace/vtk_internal/rules.bzl line 186 at r1 (raw file):

            build this module, typically loaded from a `settings.bzl` file.

    Keys of module_metaata:

typo (or https://www.youtube.com/watch?v=nbY_aP-alkw ?)

Suggestion:

metadata

tools/workspace/vtk_internal/rules.bzl line 309 at r1 (raw file):

def generate_abi_namespace():
    """Generates vtkABINamespace.h, mimicing the Common/Core/CMakeLists.txt.

typo

Suggestion:

mimicking

Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 unresolved discussions, needs at least two assigned reviewers


tools/workspace/vtk_internal/repository.bzl line 47 at r1 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

This test happens to work with the keys documented, but the overall effect is somewhat misleading.

The distinction in number is nothing to do with the incidental grammar status of the keys; its rather whether the key takes one or multiple values.

Perhaps instead:

multivalued_keys = ["DEPENDS", "PRIVATE_DEPENDS"]  # maybe others later?
if key in multivalued_keys:
    # Multi-valued key.
   # ... blah
else:
    # Single-valued key.

Or, if you are confident that no = characters will ever be embedded in values, just branch on the length of the token list?

Possibly clearer now?


tools/workspace/vtk_internal/rules.bzl line 75 at r1 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

nit does not hdrs_extra.append(module_h) work in Starlark?

At least one other instance below.

Nope. Function arguments are immutable (readonly). If I wanted to append, I'd need to copy it first.


tools/workspace/vtk_internal/rules.bzl line 92 at r1 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

This slice is a masterpiece of obfuscation. What exactly is the goal here?

Possibly clearer now?

@jwnimmer-tri
Copy link
Collaborator Author

+@SeanCurtis-TRI for platform review, please. Rico will be on-call tomorrow, and you're a good candidate for "a developer who might need to understand this stuff enough to make a small adjustment" so my goal is for you to be able to find your way around here.

Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform)

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

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

:LGTM: With a couple of typos.

Reviewed 10 of 12 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: 4 unresolved discussions


tools/workspace/vtk_internal/repository.bzl line 66 at r2 (raw file):

            result[key] = 1
        else:
            fail("{}: Too may values for {}".format(subdir, key))

nit: typo

Suggestion:

many

tools/workspace/vtk_internal/repository.bzl line 66 at r2 (raw file):

            result[key] = 1
        else:
            fail("{}: Too may values for {}".format(subdir, key))

BTW It seems that this error message is a regression against the trailing "S" heuristic. The defect may not be that there are too many values, but that there are multiple values, but we weren't told to interpret it as a list. Is it worth rephrasing the error message accordingly?


tools/workspace/vtk_internal/repository.bzl line 71 at r2 (raw file):

def create_modules_bzl(repo_ctx):
    """Finds all vtk.module files, parses them, and writes their content into
    an loadable `modules.bzl` file in the root of the repository.

nit: typo

Suggestion:

a 

tools/workspace/vtk_internal/patches/io_image_formats.patch line 28 at r2 (raw file):

   reader->Delete();
-  vtkImageReader2Factory::AvailableReaders->AddItem((reader = vtkHDRReader::New()));
+  // vtkImageReader2Factory::AvailableReaders->AddItem((reader = vtkHDRReader::New()));

BTW This will be coming back soon, because we'll want to set the envirornment map in RenderEngineVtk via .hdr files. You don't need to take any action now, I'm just flagging it to help log this into my own memory.

This commit does NOT switch Drake to use the VTK build from source.

Instead, we'll only compile vtkImageIO and then validate it with a
temporary unit test. Future work will switch the various parts of
Drake to use the VTK source build.

The goal here is to get the build system infrastructure under control,
so that it's easy to tweak and apply it down the road.
Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),SeanCurtis-TRI(platform)


tools/workspace/vtk_internal/repository.bzl line 66 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW It seems that this error message is a regression against the trailing "S" heuristic. The defect may not be that there are too many values, but that there are multiple values, but we weren't told to interpret it as a list. Is it worth rephrasing the error message accordingly?

Done

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),SeanCurtis-TRI(platform)

@jwnimmer-tri jwnimmer-tri merged commit 56874e6 into RobotLocomotion:master Aug 29, 2023
1 check was pending
@jwnimmer-tri jwnimmer-tri deleted the vtk-image-io-from-source branch August 29, 2023 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: medium release notes: fix This pull request contains fixes (no new features)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants