Repository navigation
Correct compilation bugs when heli involved - #34804
peterbarker wants to merge 3 commits into
Conversation
Each build command extended bld.options.program_group, and bld.options
is waf's global options object rather than anything owned by the build
context, so the list accumulated across the commands run by one
invocation. `./waf copter plane` therefore built copter's programs a
second time during the plane command:
ACCUM cmd=copter program_group=['copter'] (id=123652359700064)
ACCUM cmd=plane program_group=['copter', 'plane'] (id=123652359700064)
Keep the selected groups on the build context instead, so each command
builds only what it was asked for.
get_legacy_defines() decided whether to compile as heli from the
command being run: `bld.cmd == 'heli' or 'heli' in bld.targets`. That
applied to every sketch, not just ArduCopter, and copter and heli
shared one set of vehicle-dependent library objects, so whenever both
appeared in a single build only one of them could be right:
- `./waf bin` (and plain `./waf`) produced an arducopter-heli built
as APM_BUILD_ArduCopter
- `./waf copter heli` relinked arducopter against objects compiled
as APM_BUILD_Heli
- `./waf --targets bin/arduplane,bin/arducopter-heli` compiled plane
as APM_BUILD_Heli, failing to link with e.g.
undefined reference to `AP_Winch::get_singleton()'
None of these failed loudly in every case; several produced binaries
which linked cleanly and were simply not the firmware asked for.
Let ap_program() and ap_stlib() take an ap_build_type, defaulting to
the sketch name as before, and key the vehicle-dependent library
objects on it, so a sketch building more than one vehicle type gets
one set of objects per type. AP_BUILD_TARGET_NAME is still the sketch
name, so heli keeps using ArduCopter's storage file and defaults.
Heli is built from ArduCopter as APM_BUILD_Heli. Rather than relying on the waf command to choose which way the shared vehicle library is compiled, build a separate ArduCopter_heli_libs with the Heli build type, and mark the heli program the same way. This lets copter and heli be built in one invocation (or by `./waf bin`) with both binaries correct. Switching between `./waf copter` and `./waf heli` also no longer recompiles the vehicle-dependent library objects each time, at the cost of compiling them twice when building both.
Global Size Summary (Total Flash delta in bytes)Built from 0470735 rebased onto afeb9c4
|
|
Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting. Reviewed at head All three passes (primary, cold, validation) are complete and none raised a finding; there are no previous findings. The head has not moved: the fresh snapshot head, the pinned head and the CI head are all 0470735, the title is unchanged, and the only comment in the thread is the size bot's report. CI is reported passing at this head. The PR is still a draft. The PR fixes three ways a heli build could get the wrong build type. The heli program now has its own vehicle-dependent library objects, compiled as APM_BUILD_Heli (https://github.com/ardupilot/ardupilot/pull/34804/files#diff-fb3d9faf3e38f8954fb4407dea3e413b2969e6734b70beb73f4621fd653857d1R39 and https://github.com/ardupilot/ardupilot/pull/34804/files#diff-fb3d9faf3e38f8954fb4407dea3e413b2969e6734b70beb73f4621fd653857d1R54). get_legacy_defines() now takes the build type from the program rather than from the waf command (https://github.com/ardupilot/ardupilot/pull/34804/files#diff-b0dd1120bcd5f894477070525481d11fe6397381c936732c4d94f2fbb3473d95R137), and each build command keeps its own copy of the program groups (https://github.com/ardupilot/ardupilot/pull/34804/files#diff-b0dd1120bcd5f894477070525481d11fe6397381c936732c4d94f2fbb3473d95R650). VERIFIED from the primary pass's retained build logs, re-read here: at the merge base, linking a SITL build of bin/arduplane together with bin/arducopter-heli fails (rc=1, undefined references to AP_Winch and AC_PosControl, because every object was built as Heli). At head the same link passes (rc=0), with the plane and heli objects in separate library groups. On MatekF405 with --consistent-builds, the heli and copter binaries built with --targets are byte-identical at base and at head, so firmware built by build_binaries.py does not change. At the merge base, VERIFIED again in this pass: the cold pass's offline waf graph probe, re-run against the pinned worktree, exits 0. Copter and heli get distinct library variants and build types, plane stays APM_BUILD_ArduPlane, and the per-command program group selection leaves the shared options list alone. get_legacy_defines(), ap_library() and the program-group lists have no callers outside the changed code (git grep). The ESP32 CMake link only has a copter branch, which still links ArduCopter_libs, so it is unaffected. The commits follow the per-subsystem rule (waf:, waf:, Copter:), and the branch convention checker passed in the primary pass. Not exercised (UNCONFIRMED): SITL autotests (Copter and Helicopter), ESP32, Linux and QURT board builds, and a build of the intermediate waf-only commit. The repository does not require each commit to build on its own. |
Summary
Make
./waf copter heli,./waf plane copter rover sub blimp heli,./waf binand similar produce correct binaries, rather than binaries silently compiled for another vehicle type. Supersedes #34034, which made multi-firmware invocations a fatal error instead.Classification & Testing (check all that apply and add your own)
Builds are reproducible here, so I tested by byte comparison. I built each firmware alone from a clean build directory as a reference, then built from clean with the invocations below and compared every resulting binary against its reference.
Every combination of SITL firmwares. I took every subset of two or more of the seven SITL firmwares (antennatracker, blimp, copter, heli, plane, rover, sub). Each subset was built in one invocation from a clean build directory, once in alphabetical order and once reversed, which is 240 builds. That covers every pair in both orders. In each build, every requested binary had to be byte-identical to its single-firmware reference, and no other firmware could be built.
The run took 51 minutes.
Against master. The same comparison for a few invocations with master's waf files:
./waf copter heliarducopterdiffers (links fine)./waf binarducopter-helidiffers (links fine)./waf plane copter rover sub blimp heliarducopterdiffers./waf --targets bin/arduplane,bin/arducopter-heli./waf plane copterOn CubeOrange,
./waf heli coptergives.binfiles identical to separate./waf copterand./waf helibuilds.Single-firmware builds (
./waf copter,./waf heli, etc.) are byte-identical to master.tests/test_math,examples/AP_Motors_testandtool/Replaystill build.Switching between firmwares, building from clean on SITL:
./waf copter./waf heli./waf copter./waf heli./waf binnow does 2382 compiles rather than 2265, because heli's vehicle-dependent library objects are compiled separately from copter's.This PR was prepared with AI assistance (Claude Code), which traced the cause, wrote the change and ran the comparisons above; I have reviewed it.
Description
As tridge's review of #34034 pointed out, the real cause was heli. Two things combined:
The heli build type was chosen by the command, not the program.
get_legacy_defines()returnedAPM_BUILD_DIRECTORY=APM_BUILD_Heliwheneverbld.cmd == 'heli' or 'heli' in bld.targets. This applied to every sketch, not just ArduCopter. Copter and heli also shared one set of vehicle-dependent library objects, so whenever both were part of a build, one of them was compiled wrongly. For example,./waf binhas been producing anarducopter-helibuilt asAPM_BUILD_ArduCopter.Build commands accumulated each other's program groups. Each command extended
bld.options.program_group, which is waf's global options object, so the list grew across the commands of one invocation. In./waf plane copter rover sub blimp heli, the final heli command rebuilt and relinked every earlier firmware with the heli defines. That produced link failures such asundefined reference to `AP_Winch::get_singleton()'in plane. Where a binary did link, it was the wrong firmware.Changes:
waf: stop build commands accumulating each other's program groupskeeps the selected groups on the build context, so each command builds only its own programs.waf: take the vehicle build type from the program, not the commandletsap_program()andap_stlib()take anap_build_type(defaulting to the sketch name, as before). It keys the vehicle-dependent library objects on that build type, so one sketch can produce more than one vehicle type.AP_BUILD_TARGET_NAMEstays the sketch name, so heli keeps using ArduCopter's storage file and defaults.Copter: build heli against its own vehicle-dependent library objectsaddsArduCopter_heli_libswith the Heli build type and marks the heli program the same way.libraries/SITL/examples/Follow/plane_quad.shandTools/scripts/build_appveyor.sh, which build several vehicles in one invocation, keep working.Tools/scripts/build_boards.pyandTools/scripts/size_compare_branches.pybuild one vehicle per invocation because this used to break. They could be changed to build everything in one invocation, but that is left for a separate PR.