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

pydrake: Expose pybind11 version information #8118

Merged

Conversation

EricCousineau-TRI
Copy link
Contributor

@EricCousineau-TRI EricCousineau-TRI commented Feb 20, 2018

Addresses part (b) of #7856, supersedes #8107.


This change is Reviewable

@EricCousineau-TRI
Copy link
Contributor Author

+@jamiesnape for feature review, please.


Review status: 0 of 4 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

The new write_file.bzl and the pre-existing tools/workspace/generate_file.bzl probably need some de-duplication.

@EricCousineau-TRI
Copy link
Contributor Author

Done. Keeping generate_file where it is for now, can move it later if need be.


Review status: 0 of 7 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@jamiesnape
Copy link
Contributor

:lgtm:


Reviewed 8 of 8 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


bindings/pydrake/util/test/pybind11_version_test.py, line 7 at r1 (raw file):

from __future__ import print_function

import pydrake.util.pybind11_version as mut

BTW Not a great fan of mut, the meaning is not obvious without some thought, but I guess it is convention here.


Comments from Reviewable

@EricCousineau-TRI
Copy link
Contributor Author

+@sammy-tri for platform review, please.


Review status: all files reviewed at latest revision, 1 unresolved discussion.


bindings/pydrake/util/test/pybind11_version_test.py, line 7 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

BTW Not a great fan of mut, the meaning is not obvious without some thought, but I guess it is convention here.

OK I'd prefer to keep it for now, but will take note. Thanks!


Comments from Reviewable

@sammy-tri
Copy link
Contributor

:lgtm:


Reviewed 8 of 8 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@EricCousineau-TRI EricCousineau-TRI force-pushed the issue/7856_b_python branch 2 times, most recently from 83cff44 to 281b917 Compare February 21, 2018 00:13
@EricCousineau-TRI
Copy link
Contributor Author

Review status: 4 of 8 files reviewed at latest revision, all discussions resolved, some commit checks pending.


bindings/pydrake/util/BUILD.bazel, line 146 at r2 (raw file):

    name = "pybind11_version_py",
    srcs = ["pybind11_version.py"],
    # `ctx.actions.write` will always produce an exectuable file, regardless of

FYI @jwnimmer-tri Can I ask what you'd like to do here? Just always nolint any generated py files?
I checked genfiles, and all generated files are executable.


Comments from Reviewable

@EricCousineau-TRI
Copy link
Contributor Author

Review status: 4 of 8 files reviewed at latest revision, 1 unresolved discussion.


bindings/pydrake/util/BUILD.bazel, line 146 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

FYI @jwnimmer-tri Can I ask what you'd like to do here? Just always nolint any generated py files?
I checked genfiles, and all generated files are executable.

Er, that was grammatically unclear: "Should we always add tags = ["nolint"] for any generated py files?"


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

bindings/pydrake/util/BUILD.bazel, line 146 at r2 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Er, that was grammatically unclear: "Should we always add tags = ["nolint"] for any generated py files?"

OK Yeah, genfiles are always executable for some reason. Adding nolint is fine. (The longer term game, which also applies to cxx files, is to teach the linter that things in genfules shouldn't have the shebang check applied.)


Comments from Reviewable

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

Successfully merging this pull request may close these issues.

4 participants