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

Defer "gurobi not found" errors until build time #8578

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Apr 11, 2018

This is required for bazel query commands or genquery() rules to run without error. We want users without Gurobi configured to still be able to use the query features of Bazel. (This will soon become required for linters that inspect libdrake.so compositional correctness #6464.)

Do not merge until:

  • Passes macOS -everything CI.

This change is Reviewable

@jwnimmer-tri
Copy link
Collaborator Author

+@jamiesnape for feature review, please? I was going to wait to test on macOS before reviewing, but master is broken. If you're prefer to wait to review until that's fixed, that's a-ok by me too.

@jamiesnape
Copy link
Contributor

jamiesnape commented Apr 12, 2018

This is going to make CDash very happy since we can now generate a package list in advance, I think. FYI @BetsyMcPhail.

@jamiesnape
Copy link
Contributor

:lgtm: (pending CI)


Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


tools/workspace/os.bzl, line 116 at r1 (raw file):

        a struct, with attributes:
        - error: str iff any error occurred, else None
        - basic_os: str either "ubuntu" or "macos" if no error

BTW I think I would call this distribution, but I am not strongly against the current naming.


tools/workspace/gurobi/repository.bzl, line 22 at r1 (raw file):

        # unset, pass the empty string to our template() call, but symlink a
        # dummy distro path since we can't symlink to the empty string.
        gurobi_path = repo_ctx.os.environ.get("GUROBI_PATH", "")

BTW The Gurobi docs use /opt/gurobi752/linux64, so maybe in future this could look in that location as well and save an environment variable.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator Author

+@soonho-tri for platform review, please.

(Note that I'm going to wait for this to pass on macOS CI prior to merging.)


Review status: 3 of 5 files reviewed at latest revision, 1 unresolved discussion.


tools/workspace/os.bzl, line 116 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

BTW I think I would call this distribution, but I am not strongly against the current naming.

Awesome. I didn't like basic_os but was stumped for a better name.


Comments from Reviewable

@soonho-tri
Copy link
Member

:lgtm:


Reviewed 3 of 5 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@jwnimmer-tri jwnimmer-tri force-pushed the build-libdrake-gurobi-latefail branch from a5b29ec to 46d337c Compare April 12, 2018 19:13
@jwnimmer-tri
Copy link
Collaborator Author

@drake-jenkins-bot mac-sierra-clang-bazel-experimental-everything please
@drake-jenkins-bot mac-highsierra-clang-bazel-experimental please

@soonho-tri
Copy link
Member

FYI, mac CI is not happy:

ERROR: /Users/sierra/workspace/mac-sierra-clang-bazel-experimental-everything/src/tools/workspace/BUILD.bazel:60:1: Target '@gurobi//:install' is not visible from target '//tools/workspace:check_licenses'. Check the visibility declaration of the former target if you think the dependency is legitimate
ERROR: /Users/sierra/workspace/mac-sierra-clang-bazel-experimental-everything/src/tools/workspace/BUILD.bazel:54:1: Target '@gurobi//:install' is not visible from target '//tools/workspace:install_external_packages'. Check the visibility declaration of the former target if you think the dependency is legitimate

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


Comments from Reviewable

This is required for `bazel query` commands or `genquery()` rules to run
without error.  We want users without Gurobi configured to still be able
to use the query features of Bazel.  (This will soon become required for
linters that inspect libdrake.so compositional correctness.)
@jwnimmer-tri jwnimmer-tri force-pushed the build-libdrake-gurobi-latefail branch from 46d337c to 76b9141 Compare April 12, 2018 22:54
@jwnimmer-tri
Copy link
Collaborator Author

@drake-jenkins-bot mac-sierra-clang-bazel-experimental-everything please
@drake-jenkins-bot mac-highsierra-clang-bazel-experimental please

FYI, mac CI is not happy ...

Yeah; that's the downside to having two separate BUILD files, instead of emitting them with string concatenation -- its easy to accidentally have them diverge if you're not careful.

@soonho-tri
Copy link
Member

Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@jwnimmer-tri jwnimmer-tri merged commit 5c59be3 into RobotLocomotion:master Apr 13, 2018
@jwnimmer-tri jwnimmer-tri deleted the build-libdrake-gurobi-latefail branch April 13, 2018 01:24
@jamiesnape jamiesnape removed their assignment Jun 22, 2021
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.

3 participants