feat: build timestamp added at link-time to further avoid rebuilds #433
Conversation
WalkthroughScript output moved from a generated header to a compiled source: the pre-build script now emits src/version.cpp (raw string definitions), a new header Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant PIO as PlatformIO
participant Script as auto_firmware_version.py
participant FS as src/version.cpp
participant Header as src/version.h
participant CC as Compiler/Linker
Dev->>PIO: trigger build
PIO->>Script: run pre-build script
Script->>FS: write raw const char definitions (BUILD_GIT_VERSION, BUILD_TIMESTAMP)
Script->>Header: ensure header declares extern const char symbols
PIO->>CC: compile sources (build_src_filter includes version.*)
CC-->>Dev: linked firmware binary
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
platformio.ini (1)
64-64: LGTM – source filters now include version. where needed.*This confines rebuilds to version.cpp + link, as intended.
If all envs need the generated version.cpp, consider moving:
- extra_scripts = pre:scripts/auto_firmware_version.py
…to [env] to DRY, unless any env must opt out.Also applies to: 83-83, 95-95
scripts/auto_firmware_version.py (2)
6-11: Harden git describe invocation and add a fallback.Capture stderr, check return code, and fall back to short SHA if tags are absent. Also addresses the S607 hint by avoiding silent failures.
-def get_firmware_specifier(): - ret = subprocess.run(["git", "describe", "--tags", "--dirty", "--exclude", "nightly"], stdout=subprocess.PIPE, text=True) #Uses any tags - build_version = ret.stdout.strip() - print ("Build version: " + build_version) - return build_version +def get_firmware_specifier(): + ret = subprocess.run( + ["git", "describe", "--tags", "--dirty", "--exclude", "nightly"], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + ) + if ret.returncode != 0 or not ret.stdout.strip(): + fallback = subprocess.run( + ["git", "rev-parse", "--short", "HEAD"], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + ) + build_version = (fallback.stdout or "").strip() or "unknown" + else: + build_version = ret.stdout.strip() + print("Build version: " + build_version) + return build_versionOptional: resolve S607 fully by locating git explicitly:
# add at top-level import shutil git = shutil.which("git") or "git" # then use [git, "describe", ...] and [git, "rev-parse", ...]
17-22: Write to the PlatformIO src dir via env to avoid path assumptions; keep extern in the definition.Use PROJECTSRC_DIR in case the project layout changes; and keep extern on the definition to ensure external linkage for const.
-with open('src/version.cpp', 'w') as f: +with open(env.subst("${PROJECTSRC_DIR}") + "/version.cpp", "w") as f: f.write( - f"""#include <Arduino.h> -extern const String BUILD_GIT_VERSION = "{get_firmware_specifier()}"; -extern const String BUILD_TIMESTAMP = "{get_time_specifier()}"; -""") + f"""#include <Arduino.h> +extern const String BUILD_GIT_VERSION = "{get_firmware_specifier()}"; +extern const String BUILD_TIMESTAMP = "{get_time_specifier()}"; +""")If you adopt the char[] recommendation from version.h, emit arrays instead to avoid heap allocations:
#include <Arduino.h> extern const char BUILD_GIT_VERSION[] = "{get_firmware_specifier()}"; extern const char BUILD_TIMESTAMP[] = "{get_time_specifier()}";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitignore(1 hunks)platformio.ini(3 hunks)scripts/auto_firmware_version.py(1 hunks)src/version.h(1 hunks)
🧰 Additional context used
🪛 Ruff (0.13.1)
scripts/auto_firmware_version.py
7-7: Starting a process with a partial executable path
(S607)
🔇 Additional comments (1)
.gitignore (1)
11-11: LGTM – ignore the generated translation unit.Ignoring src/version.cpp is correct now that the build emits a .cpp for link-time constants.
| def get_time_specifier(): | ||
| build_timestamp = datetime.datetime.now(datetime.UTC).strftime("%Y-%m-%dT%H:%M:%SZ") | ||
| build_flag = "#define BUILD_TIMESTAMP \"" + build_timestamp + "\"" | ||
| print ("Build date: " + build_timestamp) | ||
| return build_flag | ||
| return build_timestamp |
There was a problem hiding this comment.
Fix Python 3.11-only datetime.UTC usage (breaks on older PlatformIO Python).
Use datetime.timezone.utc for broader compatibility.
def get_time_specifier():
- build_timestamp = datetime.datetime.now(datetime.UTC).strftime("%Y-%m-%dT%H:%M:%SZ")
+ build_timestamp = datetime.datetime.now(datetime.timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ")
print ("Build date: " + build_timestamp)
return build_timestamp📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_time_specifier(): | |
| build_timestamp = datetime.datetime.now(datetime.UTC).strftime("%Y-%m-%dT%H:%M:%SZ") | |
| build_flag = "#define BUILD_TIMESTAMP \"" + build_timestamp + "\"" | |
| print ("Build date: " + build_timestamp) | |
| return build_flag | |
| return build_timestamp | |
| def get_time_specifier(): | |
| build_timestamp = datetime.datetime.now(datetime.timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ") | |
| print ("Build date: " + build_timestamp) | |
| return build_timestamp |
🤖 Prompt for AI Agents
In scripts/auto_firmware_version.py around lines 12 to 15, the code uses
datetime.UTC which exists only in Python 3.11+ and breaks on older PlatformIO
Python; replace datetime.UTC with datetime.timezone.utc (and ensure
datetime.timezone is available via the existing import) so the timestamp uses
timezone-aware UTC compatible with older Python versions, then keep the same
strftime and return/print logic.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/auto_firmware_version.py (1)
6-10: Harden git invocation: check failures and avoid PATH‑only exec (Ruff S607).Add check=True and resolve git via shutil.which; fall back to "unknown" if unavailable.
def get_firmware_specifier(): - ret = subprocess.run(["git", "describe", "--tags", "--dirty", "--exclude", "nightly"], stdout=subprocess.PIPE, text=True) #Uses any tags - build_version = ret.stdout.strip() - print ("Build version: " + build_version) - return build_version + git_exe = shutil.which("git") + if not git_exe: + print("git not found; using 'unknown'") + return "unknown" + ret = subprocess.run( + [git_exe, "describe", "--tags", "--dirty", "--exclude", "nightly"], + stdout=subprocess.PIPE, + text=True, + check=True, + ) + build_version = ret.stdout.strip() or "unknown" + print("Build version: " + build_version) + return build_versionAdditional imports (outside this hunk):
import shutil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/auto_firmware_version.py(1 hunks)src/version.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/version.h
🧰 Additional context used
🪛 Ruff (0.13.1)
scripts/auto_firmware_version.py
7-7: Starting a process with a partial executable path
(S607)
🔇 Additional comments (1)
scripts/auto_firmware_version.py (1)
12-15: Fix Python 3.11‑only datetime.UTC usage.Use datetime.timezone.utc for compatibility with older PlatformIO Python.
def get_time_specifier(): - build_timestamp = datetime.datetime.now(datetime.UTC).strftime("%Y-%m-%dT%H:%M:%SZ") + build_timestamp = datetime.datetime.now(datetime.timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ") print ("Build date: " + build_timestamp) return build_timestamp
| with open('src/version.cpp', 'w') as f: | ||
| f.write( | ||
| '#pragma once\n' + | ||
| '#ifndef GIT_VERSION_H\n' + | ||
| '#define GIT_VERSION_H\n' + | ||
| get_firmware_specifier_build_flag() + '\n' + | ||
| get_time_specifier_build_flag() + '\n' | ||
| '#endif\n' | ||
| ) | ||
| f"""extern const char BUILD_GIT_VERSION[] = "{get_firmware_specifier()}"; | ||
| extern const char BUILD_TIMESTAMP[] = "{get_time_specifier()}"; | ||
| """) |
There was a problem hiding this comment.
Make generated C++ string literals robust and path/line‑ending stable.
Raw string literals avoid breakage if a tag ever contains quotes; also write to PROJECTSRC_DIR with UTF‑8 and LF.
-with open('src/version.cpp', 'w') as f:
- f.write(
- f"""extern const char BUILD_GIT_VERSION[] = "{get_firmware_specifier()}";
-extern const char BUILD_TIMESTAMP[] = "{get_time_specifier()}";
-""")
+with open(os.path.join(env.subst("$PROJECTSRC_DIR"), "version.cpp"), "w", encoding="utf-8", newline="\n") as f:
+ ver = get_firmware_specifier()
+ ts = get_time_specifier()
+ f.write(
+ f'extern const char BUILD_GIT_VERSION[] = R"({ver})";\n'
+ f'extern const char BUILD_TIMESTAMP[] = R"({ts})";\n'
+ )Additional import (outside this hunk):
import os1 participant
Minor adjustment to the work done in this commit so that changes to
version.hdon't flow through anywhere they are included and trigger a rebuild. Totally understandable if you're happy with the current approach using#definethough!Summary by CodeRabbit
New Features
Refactor
Chores