RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.32k stars 1.26k forks source link

Arguments passed to CI and documentation on choosing compiler should match #9198

Closed jamiesnape closed 5 years ago

jamiesnape commented 6 years ago

Per https://github.com/RobotLocomotion/drake/pull/8881#pullrequestreview-141683001.

EricCousineau-TRI commented 5 years ago

Per f2f with Jamie, this shouldn't really be fixed, because Bazel hashes its cache using the CC / CXX environment variable and may get false cache hits, and thus we shouldn't do this for now: https://github.com/RobotLocomotion/drake-ci/commit/1e1495b3d735f6d6c06a61d58a9f096f5263ae91

e.g. if we're on Xenial and request CC=/usr/bin/gcc, but it may pull cache artifacts from Bionic with a different version of GCC.

@jwnimmer-tri Can I ask if we can close this, and re-open if we ever decide this should change or Bazel changes its hashing mechanism? Or should it remain open if there's some workaround planned? If there's a workaround, is there a high-level overview of what that should be? (e.g. adding something to do with ~/.bazelrc.)

jwnimmer-tri commented 5 years ago

That all seems like a bit of a tangent. The intent isn't that all builds should leave CC / CXX unset, but rather that we might want to have at least one build that matches our documentation.

Our docs say to leave CC / CXX unset (except when opting-in to Clang on Ubuntu) and don't mention caching -- and indeed, that is what most of us do when building locally, so ideally CI should test that scenario. It seems to me that there's nothing preventing us from testing that only nightly (or weekly), with remote caching disabled.

So, the question seems to be whether setting up a job type that matches our documentation is a good cost-benefit trade-off for our engineer time.

I guess if the job type isn't pre-merge, then anyway a broken PR would still hit master and we'd soon all be experiencing it locally, before the nightly or weekly job found it for us.

What do you think about the trade-off?

EricCousineau-TRI commented 5 years ago

I'm definitely up for it being a Weekly job.

I think it's probably a valid trade off if it doesn't take much time to spin up the weekly job. @jamiesnape Can I ask how hard it is to do something like this?

jamiesnape commented 5 years ago

It is more logic to add, and exceptional builds are actually slightly tricky to add now that we generate builds combinatorially from a matrix of options, so it is not going to jump out of my own backlog unless the priority changes.

EricCousineau-TRI commented 5 years ago

Aye. In that case, I say we leave it at this lower priority (but open) until it becomes a pain point.

jwnimmer-tri commented 5 years ago

Given "... if the job type isn't pre-merge, then anyway a broken PR would still hit master and we'd soon all be experiencing it locally ...", I think I'd argue for closing this.

EricCousineau-TRI commented 5 years ago

SGTM.