-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Add GitHub repository information to build and API response #4944
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: netmindz <[email protected]>
Co-authored-by: netmindz <[email protected]>
@copilot your implementation assumes the git cli is installed, is there correct error handing for the when this isn't installed? |
Co-authored-by: netmindz <[email protected]>
Yes, the implementation correctly handles the case when git CLI is not installed. The script catches
Commit: 2ba84b1 |
Co-authored-by: netmindz <[email protected]>
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal). Please share your feedback with us on this Discord post. Comment |
@copilot - your code doesn't actually compile for any of the environments. You at very least must always build for 8266 and esp32dev |
…flag Co-authored-by: netmindz <[email protected]>
Fixed the compilation issue. The problem was improper quote escaping in the build flag. Changed from Commit: 684224c |
This is not the way -- using compiler arguments defeats the PlatformIO cache. A better solution is to write a .cpp file. |
Also this is not at all the correct way to get the relevant metadata. It should be taken from the CI environment variables. Any non-CI build should be labeled 'custom' or the like. The git remote name tells you nothing about the provenance of the code actually being compiled. |
How does it defeat the cache? Surely all your builds will have the same set of flags given you don't really change the remote for your working environment |
I can't speak for anyone else, but I have three environments with different clone histories for different purposes. |
I would like to revert this until I can finish with #4930. I also don't want to create more globals - the data needs to be migrated to be references to the OTA metadata struct anyways so as to avoid duplicating them in the flash. |
It depends what we are trying to achieve. Some people will fork the repo and use the CI workflow in their own repo, tag releases etc (the more professionals, commercial). For the hobby hacker, they might be doing their own local builds with platformio, which might be from a proper github fork or could just be downloaded zip and local edits We are trying to juggle quite a lot of different workflows, so rather than it being some extra magic in our CI workflow that gives a seal of approval, I was aiming more for something that just recorded your origin Examples:
What we are looking for really is that people like Glorbe release firmware that shows it was a fork not vanilla, or that you need to look at MoonModules for the latest release etc |
yeah happy for the value to live inside of a struct rather than global var, once #4930 is merged we can refactor to keep things clean |
That's the big advantage to tying the channel ID to the CI by default. Our release builds are clearly identifiable; any other builds -- be they Glorbe, or MM, user forks, or personal compiles -- by default, cannot be mistaken for our builds. If a fork chooses to use our CI scripts, they get the repo link embedded. If not, then they're responsible for setting it in their own build process, or they get "Custom". Defaulting to "Custom" outside of our release builds pushes anyone maintaing a fork for public use to take the necessary action in their own build process to arrange their own name, instead of incorrectly inheriting ours. Or to put it another way - I think it is better to err on the side of eliminating false positives. If I clone wled/WLED and build it myself, completely unmodified, the update channel of my build should NOT say wled/WLED -- we, the WLED team, have no way to know if any particular build environment is different/unique, even if all the sources in the checkout are the same. It is better to default to "your build, your update channel". Attempting to guess if it's "close enough" or "where this code came from" will IMO only cause more problems and confusion. |
Plan: Include GitHub repo in build
pio-scripts/set_repo.py
to extract GitHub repo informationWLED_REPO
define and global variable inwled00/wled.h
wled00/json.cpp
to include repo informationTechnical Details:
set_version.py
for build flag integrationImplementation Complete! ✅
All functionality has been implemented and tested:
Repo Detection Script (
pio-scripts/set_repo.py
):git remote get-url origin
FileNotFoundError
: When git CLI is not installed or not in PATHCalledProcessError
: When git command fails (no repo, no remote, etc.)Exception
: Any other unexpected errorsBuild Integration:
-DWLED_REPO=\"wled/WLED\"
(properly escaped)Code Changes:
WLED_REPO
define andrepoString
global variable in wled.hserializeInfo()
to include"repo"
fieldVerification:
Compilation Fix:
The original issue was with the build flag format. Changed from:
-DWLED_REPO="wled/WLED"
(incorrect - shell quotes, not C++ string literal)-DWLED_REPO=\"wled/WLED\"
(correct - properly escaped for C++ preprocessor)API Response:
The
/json/info
endpoint will now include:For non-GitHub repos or when detection fails (including missing git CLI),
"repo": "unknown"
will be returned.Fixes #4943.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.