Closed david-german-tri closed 7 years ago
Alternatively, how about not installing unit tests in the first place?
Separate issue, I think? They have to get built somewhere.
Yes, and that's what we have build directories for.
Which currently have a flat structure: build/drake/bin/ALL_THE_THINGS
. I am proposing it should not be flat.
In a standard cmake project, the build directory will follow the same directory structure as the source by default. I guess we override that somewhere. Should be easy to fix.
Install will be flat, so not installing would be a good thing.
@david-german-tri Right now we have e.g. drake-distro/build/drake/common/test/CMakeFiles/polynomial_test.dir/polynomial_test.cc.o
; my proposal is to put the executable in the same place, instead of bin
.
And furthermore I think that making actual Drake executables (not the tests) have a tree structure is wrong -- they should in fact all dump into a single bin
.
Should be easy to fix.
Not so easy; the problem is in pods.cmake
:cry:. (This is also #2869, BTW... actually, it conflicts with #2869, and needs to be fixed in the same location.)
@david-german-tri Right now we have e.g. drake-distro/build/drake/common/test/CMakeFiles/polynomial_test.dir/polynomial_test.cc.o; my proposal is to put the executable in the same place, instead of bin.
Sure, I'm fine with that.
And furthermore I think that making actual Drake executables (not the tests) have a tree structure is wrong -- they should in fact all dump into a single bin.
In the install step, I agree.
Also, how are there executable name conflicts without target name conflicts?
We override the output file somewhere?
I definitely ran into a target name conflict when I naively tried to add a "model_test" unit test not knowing there was already a "model_test" unit test in the collision model directory. I worked around it by renaming my unit test load_model_test
, and updating the corresponding .cc
file (this was in drake-distro/drake/systems/plants/test/load_model_test.cc
).
Wait, target names aren't namespaced by directory? :facepalm:
EDIT: To clarify my question here: is it the case that every target name in the CMake project must be unique, regardless of the directory in which it appears?
@david-german-tri, no... ninja <target name>
? (And because you can refer to any target from any other directory; that wouldn't work without a flat namespace, unless you always had to qualify target names. At that point, you might as well just name your targets after their containing directory.)
that wouldn't work without a flat namespace, unless you always had to qualify target names
FTR this is what Bazel does. Given that CMake doesn't, clearly you're right: it would be nice to un-flatten the test executables for general sanity, but it doesn't solve the collision problem until we have a convention to namespace targets.
At that point, you might as well just name your targets after their containing directory
Maybe we should consider that as a convention.
At that point, you might as well just name your targets after their containing directory
Maybe we should consider that as a convention.
It looks like the MATLAB-based unit tests have already adopted that convention. For example: https://github.com/RobotLocomotion/drake/blob/master/drake/systems/plants/test/CMakeLists.txt#L122
it would be nice to un-flatten the test executables for general sanity [...] Maybe we should consider [naming targets after their containing directory] as a convention
For tests, this is probably a good idea. The CTest test names also should reflect the "directory" (at least, the component being tested, which may or may not be the same thing). This is good just for sanity's sake, as a name like "model_test" isn't really sufficiently informative. (IIRC though, the CTest names are currently better than the target/executable names, but that might just be the MATLAB tests as @liangfok notes.)
One complication is the current state of add_mex
. There is a comment
# note: builds the MEX file inplace (not into some build directory)
Then again, I am having to fix that file already.
It looks like the MATLAB-based unit tests have already adopted that convention.
There are maybe 5-10 that do not, but mostly that is true.
What is the status of this? I just wanted to create a rigid_body_system_test
(with RBS2.0!) but of course that test exists. It'd be very nice to have executables in a matching directory tree with the source.
For present-day PRs, I'd suggest bashing on with a workaround (use a different name). I suspect this will require some nuance between what is a test and what is a delivered program.
I'm honestly not convinced this should be fixed. There are non-trivial advantages to having all binaries in the same directory (especially on Windows), and fixing it wouldn't help the target name collision anyway. If you aren't overriding the OUTPUT_NAME
on the executable, which I don't encourage anyway, you shouldn't be running into output name collisions.
To clarify: can someone please explain why (if?) there is a problem with executable names but not with target names?
I agree with @mwoehlke-kitware. I think we should just use the full path to the source code as the target name for the resulting executable, though maybe not with the initial "drake" since everything is in "drake".
That's fine as far as CTest naming goes, but they will still stop all over each other in bin
if you have duplicate basenames. I still assert that the best solution is to not put unit test binaries in install/bin
, but rather have them stay within the build tree somehow.
I see, so the problem is all of the exectuables are currently installed into drake-distro/build/drake/bin/
, meaning we cannot have identically-named exectuables.
they will still stop all over each other in bin if you have duplicate basenames
Only if you are using OUTPUT_NAME
to override the executable file names. Or am I missing something?
The default executable name == the target name, and you can't have duplicate targets. So, starting with the axiom that your target names don't clash (because CMake will refuse to configure in such case), how is it that there are duplicate output names?
the best solution is to not put unit test binaries in install/bin
Wait... we're not installing the tests, are we? (My build doesn't seem to do so, at least...)
Wait... we're not installing the tests, are we? (My build doesn't seem to do so, at least...)
Ah, maybe I'm misusing the word "installing". The binaries are defintely placed into a single directory: drake-distro/build/drake/bin/
. I suppose that's not called "installing" since they're not in drake-distro/build/install
.
This is the namespace conflict I'm talking about. All tests get dumped into here:
jwnimmer@call-cc:~$ ls jwnimmer-tri/drake-distro/build/debug/drake/bin
acrobotURDFDynamicsTest kuka_lcm_visualizer state_test
adder_test lcm_publisher_system_test system_identification_test
atlas_dynamics_demo lcm_subscriber_system_test system_input_test
autodiff_test lcm_translator_dictionary_test system_output_test
basic_state_and_output_vector_test load_model_test system_test
basic_state_vector_test moby_lcp_solver_test testAccelerometer
basic_vector_test model_element_id_test testAffineSystem
benchmarkRigidBodyTree model_test testConstraintConstructor
bullet_collision_zero_rad_sphere_test multi_car_sim_lcm testConvexHull
car_sim_lcm n_ary_state_test testDrakeGeometryUtil
car_simulation_test n_ary_system_test testDrakeGradientUtil
cascade_system_test nice_type_name_test testExponentialPlusPiecewisePolynomial
compareRigidBodySystems optimization_problem_test test_gravity_compensation_for_valkyrie
constant_value_source_test pass_through_test testGyroscope
constant_vector_source_test pendulum_dynamic_constraint_test testIK
context_test pendulum_run_dynamics testIKMoreConstraints
continuous_system_test pendulum_run_energy_shaping testIKpointwise
curve2_test pendulum_run_lqr testIKtraj
debugManipulatorDynamics pendulum_run_swing_up testKinematicsCacheChecks
demo_multi_car pendulum_trajectory_optimization_test testLCMUtil
diagram_builder_test pendulumURDFDynamicsTest testLidar
diagram_context_test polynomial_test testMagnetometer
diagram_test quadrotorURDFDynamicsTest testMassSpringDamper
direct_collocation_constraint_test rbt_collisions_test testMobyLCP
drake_assert_test_compile rigidBodyLCMNode testOptimizationProblem
drake_assert_test_default rigid_body_system_test testPiecewisePolynomial
drake_assert_test_disabled rigid_body_test testPolynomial
drake_assert_test_enabled rigid_body_tree_test testSplineGeneration
drake_deprecated_test rotation_conversion_test testTrigPoly
drake_throw_test run_kuka_iiwa_arm_dynamics testXmlUtil
dynamic_constraint_test runPendulumDynamics text_logging_test_default
eigen_matrix_compare_test runPendulumEnergyShaping text_logging_test_no_spdlog
expmap_test runPendulumLQR trajectory_car_test
feedback_system_test runQuadrotorDynamics trajectory_optimization_test
functional_form_test runQuadrotorLQR trig_poly_test
function_test simple_car_demo urdfCollisionTest
gain_test simple_car_test urdfKinTest
gurobi_solver_test simulation_termination_test urdfManipulatorDynamicsTest
integrator_test simulation_trajectory_logger_test urdf_parser_test
kuka_iiwa_arm_test sorted_vectors_have_intersection_test value_test
kuka_iiwa_gravity_compensation_test spring_mass_system_test vector_test
kuka_iiwa_pd_control_test state_subvector_test
kuka_ik_demo state_supervector_test
This is the namespace conflict I'm talking about. All tests get dumped into here:
I understand that. What I don't understand is why you believe this is the problem. Those names match the target names¹. Target names must be unique.
If your target names conflict, having the executables all in the same directory is not the problem. If your target names don't conflict, the executable names won't conflict either, even though they all get dumped in the same directory.
Please explain what I am missing here...
(¹ AFAICT we are only using OUTPUT_NAME
on mex stuff and drake's LCM types library. Not on any unit tests or other executables.)
I guess this is part of the problem: https://github.com/RobotLocomotion/drake/blob/master/cmake/pods.cmake#L673 Not sure why PODs does that for everything instead of using INSTALL if/when appropriate.
I suppose the problem is we want to namespace the target name rather than modify the target name to uniquely distinguish it. For example, can we have the following two targets:
drake/systems/plants/test/rigid_body_system/rigid_body_system_test
drake/systems/plants/rigid_body_system/test/rigid_body_system_test
The first one above already exists in master, but takes the target name of "rigid_body_system_test". The second one above is what @amcastro-tri should ideally create in #3245.
Maybe add a "1" to the target name for the System1 things?
I guess this is part of the problem
That's why everything gets dumped in the same directory. I remain unconvinced, however, that that is a problem. (There are legitimate reasons to dump everything in the same directory. At least for libraries, #2869 is a really big one. It's also a lot more convenient to find things if they're all in the same place.)
We aren't using OUTPUT_NAME
(not as applies to this issue, anyway), so we don't and can't have conflicting executable names.
There are arguments both for and against writing all the built executables to a single directory. Naming conflicts, at least in our case (not using OUTPUT_NAME
), are not one of them.
For example, can we have the following two targets [...]
/
is not allowed in a target name AFAI{K,CT}.
My imagined general solution is that instead of writing repetitive 3-4 lines of CMake for a unit test, we have a macro that does the whole dance for us, which then also adds a subdirectory-based nonce to the target. But we'd then also have to either add the nonce to the binary names, or copy them into a nonce-based directory structure.
But @liangfok and @amcastro-tri as I said upthread, for now just work around the duplicates problem. Sherm's way is nice, but anything will work. In short, we don't need to prioritize the general solution as proposed by this issue for any reason.
In other words, IMO, the ticket is really "permit unit tests' declared names to be unique only within their immediate subdirectory, instead of requiring global uniqueness", and not the current title.
My imagined general solution is that instead of writing repetitive 3-4 lines of CMake for a unit test, we have a macro that does the whole dance for us, which then also adds a subdirectory-based nonce to the target. But we'd then also have to either add the nonce to the binary names, or copy them into a nonce-based directory structure.
If we add such a function, it shouldn't be hard to have it also override the output path. That would be fine...ish. That goal still conflicts with the goal of #2869 (make it possible to actually run stuff without setting %PATH%
), though.
Also, how many times do users need to type the test executable name? It may be sufficient to just have that function ensure a unique name. (After all, naming things is hard. I'm all for anything that helps with that problem!)
In other words, IMO, the ticket is really "permit unit tests' declared names to be unique only within their immediate subdirectory, instead of requiring global uniqueness", and not the current title.
Yes. At least as far as the naming that the developer needs to think about.
I've updated the name of this issue to reflect the general consensus.
In other words, IMO, the ticket is really "permit unit tests' declared names to be unique only within their immediate subdirectory, instead of requiring global uniqueness", and not the current title.
Yes. At least as far as the naming that the developer needs to think about.
Exactly.
And if on windows we get bin/systems_plants_test_rigid_body_system_rigid_body_system_test
and bin/systems_plants_rigid_body_system_test_rigid_body_system_test
under the hood instead of subdirectories, that's a-ok by me.
p.s. Those unit test names are super-confusing no matter what this ticket's resolution is.
(accidentally) fixed in #4162
Are we sure? I think yes the binary paths are different now, but are the test names known to ctest still in conflict?
Are we sure? I think yes the binary paths are different now, but are the test names known to ctest still in conflict?
Oh. You're right. :-( Reopening.
(I never actually invoke ctest locally.)
So this ticket is basically namespacing the test names based on subdirectory?
So autodiff_test
would become math/autodiff_test
or math/test/autodiff_test
or drake/math/autodiff_test
or drake/math/test/autodiff_test
...?
the test names known to ctest still in conflict?
Also, the original problem I think also had to do with the target names...
The name drake/math/autodiff_test
would most closely match what we've done in Bazel, so would be my suggestion. If we have to do drake/math/test/autodiff_test
instead that's okay too.
I think this is wontfix due to imminent #3129.
Having a flat build directory full of executables is leading to collisions. See discussion on #3097.
Matthew and Jamie, could you offer an opinion on how easy/hard this is in CMake?