Skip to content

Conversation

@tpwrules
Copy link
Contributor

@tpwrules tpwrules commented Sep 14, 2025

This removes a wart in the build system where environment variables from the hwdef generator were written to a Python Pickle file (not a Python source file as the name suggests) then read back in. This cleans that up to just pass the data directly. Unlike the implication in the source code, it was never really possible to use this file to add custom environment variables as it is unconditionally overwritten every compile. Please see the commits for details.

It also uncovers some more warts (for which fixes will be PRed later)

  • The whole load_env_vars thing needs rethinking, it was not optional for a long time and can probably be cleaned up
  • There should be no need to rerun the hwdef generator at build time, waf has mechanisms to preserve files

Tested that there is no compiler output change for an arbitrary board for each of ChibiOS, ESP32, and Linux.

These are already loaded right after the initial generation, and waf
ensures `cfg.env` persists to `bld.env`, so loading them again just
mixes the values in again for little effect.
Running it during the build just wastes time as it is already checked
for and rerun if necessary during pre_build. For Linux and ESP32 the
build run was stubbed out (`rule=""`) anyway.

Also allows de-environmentification of `BOOTLOADER_OPTION` as that was
only ever used during the unnecessary and now deleted build run.
Read from the passed in variable instead of the global args object. By
default the variable is set to the args object's contents so there is no
change in behavior.

Note that much of this logic is fishy and/or wrong; that is preserved
and left to be fixed another day.
Avoids time and complexity of a subprocess and matches esp32 and linux.
Avoid the need to go through `env.py`. Also adjust comments on the
relevant functions as this is all they could have been used for for a
long time now, as the file is overwritten each time the generator runs.

import hal_common

# sys.path already set up at the top of boards.py
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably set it up again in case we move things around over there.

I don't think it will hurt

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand it will immediately explode if that happens so it will be easy to fix but I see your point.

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do like the look of this, but we do need someone more familiar with this process to review it.

No-compiler-output-change is pretty compelling....

@tpwrules
Copy link
Contributor Author

tpwrules commented Sep 22, 2025

I welcome attempts to break it changing targets and bootloaders and cleaning build dirs and etc. Just so long as it's not also broken in the same way in master :)

@peterbarker
Copy link
Contributor

Board                    AP_Periph  blimp  bootloader  copter  heli  iofirmware  plane  rover  sub
CubeOrange-periph-heavy  *                 *
Durandal                            *      *           *       *                 *      *      *
Hitec-Airspeed           *                 *
KakuteH7-bdshot                     *      *           *       *                 *      *      *
MatekF405                           *      *           *       *                 *      *      *
Pixhawk1-1M-bdshot                  *                  *       *                 *      *      *
SITL_x86_64_linux_gnu               0                  0       0                 0      0      0
f103-QiotekPeriph        *                 *
f303-Universal           *                 *
iomcu                                                                *
revo-mini                           *      *           *       *                 *      *      *
skyviper-v2450                                         *

@tridge
Copy link
Contributor

tridge commented Oct 1, 2025

@tpwrules does this have any impact for multi-target waf builds? eg. "./waf plane copter" or "./waf all"

@peterbarker
Copy link
Contributor

So long as we don't cause any regressions, this is a welcome change!

@tpwrules
Copy link
Contributor Author

tpwrules commented Oct 4, 2025

@tpwrules does this have any impact for multi-target waf builds? eg. "./waf plane copter" or "./waf all"

Tested both and all binaries match before and after. Note that we do the latter in CI.

@tridge tridge merged commit 1d92d6f into ArduPilot:master Oct 5, 2025
111 of 112 checks passed
@tpwrules tpwrules deleted the pr/kill-env-py branch October 5, 2025 21:03
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.

4 participants