Skip to content
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

feat: commit hash #228

Merged
merged 10 commits into from
Oct 13, 2023
Merged

feat: commit hash #228

merged 10 commits into from
Oct 13, 2023

Conversation

unlogisch04
Copy link
Contributor

@unlogisch04 unlogisch04 commented Feb 6, 2023

Change

This pull request adds the Git commit hash in the built firmware.

Notes:

The web flasher needs a new implementation, this should not be merged without the change.

⚠️ Needs testing ⚠️

@unlogisch04 unlogisch04 marked this pull request as ready for review February 6, 2023 08:14
@ButterscotchV
Copy link
Member

Added this branch to my site (https://slimevr-firmware.bscotch.ca/) as unlogisch04/feat_commitid to be used for testing.

I can confirm this breaks on the web firmware tool. Check the attached log for more info:
firmware-tool-log.log

Copy link
Member

@ButterscotchV ButterscotchV left a comment

Choose a reason for hiding this comment

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

This shouldn't crash if not in a git repo (ex. downloading a release zip, etc), even though it's not much of an expected case, it's good to keep it working anyways. Maybe a big warning instead?

For firmware tool compatability, support should be added for reading the commit ID from an environment variable, as we have the metadata but no way to pass it in.

@unlogisch04
Copy link
Contributor Author

Oh i did forgot, that it can be out of a git repo.
Thanks for testing.

@unlogisch04 unlogisch04 marked this pull request as draft March 14, 2023 22:53
@TheDevMinerTV TheDevMinerTV marked this pull request as ready for review April 30, 2023 01:27
@TheDevMinerTV TheDevMinerTV changed the title Feat commitid feat: commit hash Apr 30, 2023
@TheDevMinerTV
Copy link
Member

@ButterscotchV could you check again? I've added error handling to the script

@ButterscotchV
Copy link
Member

Tested again, doesn't compile

File "scripts/get_git_commit.py", line 22
    print(f"'-DGIT_REV=\"{revision}\"'")
                                      ^
SyntaxError: invalid syntax

firmware-tool-log.log

@nekomona
Copy link
Contributor

nekomona commented Jul 4, 2023

Tested again, doesn't compile

File "scripts/get_git_commit.py", line 22
    print(f"'-DGIT_REV=\"{revision}\"'")
                                      ^
SyntaxError: invalid syntax

firmware-tool-log.log

That could be from an old version of python, f-strings are supported from python 3.6. Working fine with ubuntu and PlatformIO installed last month

Copy link
Contributor

@nekomona nekomona left a comment

Choose a reason for hiding this comment

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

Tested with these two patches with and without git repo

@TheDevMinerTV TheDevMinerTV requested a review from nekomona July 4, 2023 20:45
@ButterscotchV
Copy link
Member

Oh also I've tested and it does appear to compile. I've added support for the GIT_REV environment variable on my web firmware tool fork, so that can also be tested hopefully (I hope I did it right -w-).

TheDevMinerTV and others added 2 commits July 5, 2023 18:27
Co-authored-by: nekomona <nekomona@163.com>
Co-authored-by: nekomona <nekomona@163.com>
Copy link
Member

@ButterscotchV ButterscotchV left a comment

Choose a reason for hiding this comment

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

Works on web firmware tool perfectly 👍

Copy link
Member

@TheDevMinerTV TheDevMinerTV left a comment

Choose a reason for hiding this comment

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

LGTM

@Eirenliel please merge

@Eirenliel Eirenliel merged commit 14f2752 into SlimeVR:main Oct 13, 2023
@unlogisch04 unlogisch04 deleted the feat_commitid branch May 6, 2024 19:16
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.

5 participants