Cleanup build script + fix -v#27
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
I will take some time to review this. |
|
LGTM but I will ask copilot to look if I missed anything. |
There was a problem hiding this comment.
Pull request overview
This PR aims to improve release/build packaging and version reporting by wiring build-time version/package metadata into the application, refreshing the AppImage tooling version, and documenting a known WebKitGTK/AppImage runtime limitation.
Changes:
- Export
VERSION/PACKAGE_NAMEin the release build script and pass them into the C++ binary via CMake compile definitions to fix-vreporting. - Update linuxdeploy AppImage version/SHA and simplify parts of the AppImage build script.
- Add a “Known issues” section to the README describing a WebKitGTK/AppImage limitation and workarounds.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
src/main.cpp |
Uses build-provided version/package values for --version and desktop file name. |
scripts/build-release.sh |
Exports version/package variables and updates linuxdeploy version/SHA. |
CMakeLists.txt |
Pulls env-provided metadata into compile-time definitions for the build. |
README.md |
Documents a known WebKitGTK/AppImage runtime issue and suggested workarounds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| set(VERSION "$ENV{VERSION}") | ||
| set(PACKAGE_NAME "$ENV{PACKAGE_NAME}") |
There was a problem hiding this comment.
Yeah maybe we should fix this but I don't like the Copilot suggestion. Any thoughts on this one? @tectrixdev
There was a problem hiding this comment.
Honestly, I think we should trust our build script and put the environment variable in the requirements for building, as falling back to a non-existent version is a bit weird behavior for user builds. We could also source from git and use commits as versions or tags but that'd be a bit to complicated for this small of an issue in my opinion. As for the package name, maybe this isn't a bad suggestion, or we should keep it static as the human readable name isn't dynamic anyways.
| add_compile_definitions(VERSION="${VERSION}") | ||
| add_compile_definitions(PACKAGE_NAME="${PACKAGE_NAME}") |
There was a problem hiding this comment.
As in fine with the proposed solution or how it is now?
| export VERSION="${1:-0.1.0}" | ||
| export PACKAGE_NAME="safe-exam-browser" |
There was a problem hiding this comment.
I don't think this is a problem, as with version it is an argument given to the script, but package name is specified in this script or in an environment variable, hence it wouldn't need a fallback as it is defined just fine.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
Allright, I'll fix the issues in a moment. Thanks for pointing them out! |
Summary
Verification
(the version numbers on my fork are all over the place but it is correct in this image.)