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

Add IPOPT to the Bazel build #4905

Merged
merged 3 commits into from
Jan 26, 2017

Conversation

david-german-tri
Copy link
Contributor

@david-german-tri david-german-tri commented Jan 25, 2017

Linux only for now, I'm timeboxing the toxic swamp of OS X problems.


This change is Reviewable

@david-german-tri
Copy link
Contributor Author

@rpoyner-tri for feature review

@rpoyner-tri
Copy link
Contributor

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


tools/BUILD, line 70 at r1 (raw file):

    name = "apple",
    values = {"cpu": "darwin"},
)

final newline please.


tools/ipopt.BUILD, line 14 at r1 (raw file):

# find include/coin -name "*.h" -o -name "*.hpp" -o -name "*.hdd" | sort |
# sed 's/$/",/g'| sed 's/^/"/g'
IPOPT_HDRS = [

BTW, I don't know if it's defectable yet, but please run this file through buildifier.sh. It changes some things, but the result works and does not appear to wreck the .a file ordering below.


Comments from Reviewable

@david-german-tri
Copy link
Contributor Author

Review status: 6 of 8 files reviewed at latest revision, 2 unresolved discussions.


tools/BUILD, line 70 at r1 (raw file):

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

final newline please.

Done.


tools/ipopt.BUILD, line 14 at r1 (raw file):

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

BTW, I don't know if it's defectable yet, but please run this file through buildifier.sh. It changes some things, but the result works and does not appear to wreck the .a file ordering below.

Done.


Comments from Reviewable

@rpoyner-tri
Copy link
Contributor

:lgtm:


Review status: 6 of 8 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@david-german-tri
Copy link
Contributor Author

-(status: do not merge) -(status: do not review)

+@jwnimmer-tri for platform review


Review status: 6 of 8 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@rpoyner-tri
Copy link
Contributor

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


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

Checkpoint. I still need to read the ipopt.BUILD, but looks good on the rest.


Reviewed 6 of 8 files at r1, 1 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

Reviewed 1 of 2 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


tools/ipopt.BUILD, line 110 at r2 (raw file):

    srcs = glob([
        "*",
        "**/*",

BTW I am surprised that this second line doesn't subsume the first?


tools/ipopt.BUILD, line 134 at r2 (raw file):

    visibility = ["//visibility:public"],
    alwayslink = 1,
)

BTW For other externals with a clear single output library, we have been providing a :lib target by convention (this is what our pkg-config repository rule does, for example). Maybe add an https://bazel.build/versions/master/docs/be/general.html#alias here for :lib?


Comments from Reviewable

@david-german-tri
Copy link
Contributor Author

Review status: 6 of 8 files reviewed at latest revision, 2 unresolved discussions.


tools/ipopt.BUILD, line 110 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW I am surprised that this second line doesn't subsume the first?

Done.


tools/ipopt.BUILD, line 134 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW For other externals with a clear single output library, we have been providing a :lib target by convention (this is what our pkg-config repository rule does, for example). Maybe add an https://bazel.build/versions/master/docs/be/general.html#alias here for :lib?

Done.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

Still doing some local testing, but so far so good.


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


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

:lgtm:


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


tools/ipopt.BUILD, line 87 at r3 (raw file):

    "lib/libipoptamplinterface.a",
    "lib/libcoinmumps.a",
    "lib/libcoinmetis.a",

We should not be using Metis due to licensing. I'm okay merging this PR to just match CMake -- iff there's a TODO comment added here citing #4913 and a note posted to 4913 reminding us to fix this BUILD file also.


Comments from Reviewable

@rpoyner-tri
Copy link
Contributor

Review status: all files reviewed at latest revision, 2 unresolved discussions.


drake/solvers/BUILD, line 325 at r3 (raw file):

        "test/mathematical_program_test_util.h",
        "test/nonlinear_program_test.cc",
        "test/optimization_examples.cc",

BTW are we building this module twice now -- here and in the prior test rule?


Comments from Reviewable

@rpoyner-tri
Copy link
Contributor

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


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

Review status: all files reviewed at latest revision, 2 unresolved discussions.


drake/solvers/BUILD, line 325 at r3 (raw file):

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

BTW are we building this module twice now -- here and in the prior test rule?

Yup. (Though not a new defect as of this PR.)


Comments from Reviewable

@david-german-tri
Copy link
Contributor Author

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


tools/ipopt.BUILD, line 87 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

We should not be using Metis due to licensing. I'm okay merging this PR to just match CMake -- iff there's a TODO comment added here citing #4913 and a note posted to 4913 reminding us to fix this BUILD file also.

Done.


Comments from Reviewable

@jwnimmer-tri
Copy link
Collaborator

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@david-german-tri
Copy link
Contributor Author

Not sure if the bazel failure is real.

@drake-jenkins-bot linux-gcc-bazel-experimental please

@jwnimmer-tri jwnimmer-tri merged commit b7bb572 into RobotLocomotion:master Jan 26, 2017
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