Skip to content

Conversation

@rosacry
Copy link

@rosacry rosacry commented Dec 19, 2025

Summary

Fixes a bug in the libsafety SConscript where the loop variable build_env is ignored, causing both regular and mutation testing builds to use the same environment.

The Bug

The loop iterates with build_env alternating between env and menv, but the body always uses env:

for build_env, suffix in ((env, ""), (menv, "_mutation")):
    safety = env.SharedObject(...)    # Always uses `env`
    libsafety = env.SharedLibrary(...)

This means libsafety_mutation.so should use menv (clang-17, Mull plugin, coverage flags) but incorrectly uses env.

The Fix

Use the loop variable build_env instead of hardcoded env:

safety = build_env.SharedObject(f"safety{suffix}.os", "safety.c")
libsafety = build_env.SharedLibrary(f"libsafety{suffix}", [safety])

Files Changed

  • opendbc/safety/tests/libsafety/SConscript - Use build_env instead of env in loop body

Copilot AI review requested due to automatic review settings December 19, 2025 22:27
@github-actions github-actions bot added the car safety vehicle-specific safety code label Dec 19, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a bug in the libsafety SConscript build configuration where the loop variable build_env was being ignored, causing both the regular and mutation testing builds to incorrectly use the same environment (env) instead of their respective environments.

Key Changes:

  • Use build_env instead of hardcoded env in SharedObject and SharedLibrary calls within the build loop
  • Remove hardcoded .so extension from SharedLibrary call (SCons adds platform-appropriate extension automatically)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

libsafety = build_env.SharedLibrary(f"libsafety{suffix}", [safety])

# GCC note file is generated by compiler, allow scons to clean it up
env.SideEffect("safety.gcno", safety)
Copy link

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

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

The SideEffect declaration still uses env and is inside the loop, which causes issues. On the second loop iteration when build_env == menv, the safety variable refers to the mutation build object, but this line declares that the GCC coverage file safety.gcno is a side effect of it, which is incorrect since the mutation build uses clang-17 and won't generate .gcno files. This should either be moved outside the loop after the loop completes, or be conditional on build_env == env, or use build_env if both environments can generate side effect files that need cleaning.

Suggested change
env.SideEffect("safety.gcno", safety)
if build_env == env:
build_env.SideEffect("safety.gcno", safety)

Copilot uses AI. Check for mistakes.
@rosacry rosacry force-pushed the fix-sconscript-build-env branch from 9aa8e65 to c77d456 Compare December 19, 2025 22:34
@rosacry rosacry closed this Dec 19, 2025
@rosacry rosacry deleted the fix-sconscript-build-env branch December 19, 2025 22:38
@rosacry
Copy link
Author

rosacry commented Dec 19, 2025

What looked like a bug was actually working code with implicit CI dependencies. My bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

car safety vehicle-specific safety code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant