search

maliput / delphyne

Scenario and search based Ego/Ado Car traffic simulations
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

Write a script to make sure our pybind11 hash is up-to-date #267

Closed clalancette closed 6 years ago

clalancette commented 6 years ago

As suggested in https://github.com/RobotLocomotion/drake/issues/7856#issuecomment-368143583, we should probably write a script that makes it easy to ensure that we are up-to-date with respect to the pybind11 fork that drake is using. Once we have the script, we could run it manually or in CI to make sure we are up-to-date.

EricCousineau-TRI commented 6 years ago

@apojomovsky Follow-up from #266: I was just thinking of something simple (not specifically related to #266), that just parses the *.bzl or reads from pydrake.util.pybind11_version and pukes if delphyne.repos doesn't have the same commit (and perhaps with an option to update it automatically?).

apojomovsky commented 6 years ago

Sure @EricCousineau-TRI . We'll be addressing this issue shortly.

apojomovsky commented 6 years ago

Hi @clalancette , I’ve been briefly looking at this issue and reminded me about delphyne’s current situation with drake: we are not currently pointing to drake master in the delphyne.repos file, which would make this solution pointless. Now, after having checked that our current delphyne master breaks with latest drake master, I would like to ask: Are you aware of any major blocker or known difficulty regarding it? Would you like me to create a ticket in delphyne (and address it) to get back to drake master? I would address it soon, since it’s currently a blocker for this particular issue.

clalancette commented 6 years ago

Are you aware of any major blocker or known difficulty regarding it?

Yeah, there is a blocker, unfortunately. It turns out that drake somewhat recently changed (end of January) how the ignition-math headers are installed. Because of that, trying to build ignition-rendering against newer versions of drake always failed. There are a number of ways to address this:

  1. The "correct" way, but the hardest way, is to finish porting ignition-rendering to ign-cmake. I believe (but haven't verified) that this will fix the issue. It should be possible to test this by checking out the ign-cmake branch from ignition-rendering, and see if it builds against latest drake. @caguero had been working with Ian on doing that, and I know that they got pretty far, but I don't think it is merged yet. It is probably worth talking to Carlos about that again to see if we can get that work merged.
  2. Change drake to put the ignition-math headers back to where they were before.
  3. Fix the current, non-ign-cmake version of ign-rendering to work with the new drake installation.

Would you like me to create a ticket in delphyne (and address it) to get back to drake master? I would address it soon, since it’s currently a blocker for this particular issue.

Please do file a bug, and if you have time to do some of the above, that would be great.

apojomovsky commented 6 years ago

Currently blocked by #275. We need to wait here until we move back to drake master so that we can fetch the (updated) version of pybind11 that it's currently being targeted.

EricCousineau-TRI commented 6 years ago

After speaking with @stonier and @jamiesnape, it seems like a better option would be to reinstate the pybind11 installation artifacts in drake (so that this problem can be avoided in other projects like Russ's Underactuated course) - see https://github.com/RobotLocomotion/drake-shambhala/pull/95 for a related discussion.

Would it terribly inconvenience y'all for us to revert https://github.com/RobotLocomotion/drake/pull/7805 to once again install the pybind11 artifacts?

clalancette commented 6 years ago

I think that would be a better solution for us overall, so I'm all for it. Note that due to an unrelated bug (https://github.com/ToyotaResearchInstitute/delphyne/issues/275), we are going to have to wait to get back to drake master for a bit.

basicNew commented 6 years ago

Thanks a lot for keeping an eye on this @EricCousineau-TRI . Totally agree with what @clalancette mentioned.

basicNew commented 6 years ago

PR that adds back the pybind11 package in Drake https://github.com/RobotLocomotion/drake/pull/8264

basicNew commented 6 years ago

@apojomovsky is this still required?

apojomovsky commented 6 years ago

Now that Drake is exporting his copy of pybind11 in the installation, it is no longer necessary. So I'm closing it.