-
Notifications
You must be signed in to change notification settings - Fork 2.4k
enhancement: fix EGL_BAD_PARAMETER bug for local dev builds #5706
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
enhancement: fix EGL_BAD_PARAMETER bug for local dev builds #5706
Conversation
Part of janhq#5641 - pulls fix for EGL_BAD_PARAMETER bug out of the github release workflow and into the make/yarn build process - implements a wrapper script that pins linuxdeploy and injects a new location for XDG_CACHE_HOME into the build pipeline, allowing manipulating .cache/tauri without tainting the hosts .cache
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed everything up to a384721 in 2 minutes and 16 seconds. Click for details.
- Reviewed
83lines of code in4files - Skipped
1files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/template-tauri-build-linux-x64.yml:150
- Draft comment:
Removed linuxdeploy pinning from the workflow. Ensure the shim script is always invoked for Linux builds to maintain the correct version. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. Makefile:85
- Draft comment:
Added removal of the local .cache directory. Confirm this cleanup won’t remove any needed cached data. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. package.json:33
- Draft comment:
The new 'build:tauri:linux' script now uses the shim-linuxdeploy.sh wrapper. Verify that omitting extra steps (e.g. install:cortex) on Linux is intentional. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. src-tauri/build-utils/shim-linuxdeploy.sh:4
- Draft comment:
Typographical error: 'vairables' should be corrected to 'variables'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The review rules state not to make purely informative comments or comments about unimportant issues. Typographical errors in comments are generally not actionable unless they cause confusion or are in documentation that is user-facing. This typo does not affect code logic or functionality. The rules prioritize code changes over minor comment corrections. Perhaps correcting typos in comments could improve code quality and readability, but the rules seem to discourage such minor, non-actionable comments. However, if the typo is egregious or misleading, it might be worth correcting. In this case, the typo is minor and does not impact understanding. The rules are clear that such comments should not be made unless they are actionable and necessary. The comment should be deleted because it is about a minor typo in a comment and does not require a code change. It is not actionable or important per the review rules.
Workflow ID: wflow_DFtCOTdll7dMz0iT
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
|
||
| mkdir -p "$XDG_CACHE_HOME/tauri" | ||
|
|
||
| if [ ! -f $LINUXDEPLOY ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quote the variable when testing file existence to prevent word splitting (e.g., use "[ ! -f "$LINUXDEPLOY" ]").
| if [ ! -f $LINUXDEPLOY ]; then | |
| if [ ! -f "$LINUXDEPLOY" ]; then |
|
|
||
| if [ ! -f $LINUXDEPLOY ]; then | ||
| GLOB_PATTERN="$XDG_CACHE_HOME/tauri/linuxdeploy-*-x86_64.AppImage" | ||
| rm $GLOB_PATTERN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using 'rm -f' and quoting the glob (e.g., rm -f "$GLOB_PATTERN") to avoid errors if no files match.
| rm $GLOB_PATTERN | |
| rm -f "$GLOB_PATTERN" |
| chmod a+x "$LINUXDEPLOY" | ||
| fi | ||
|
|
||
| rm $SYMLINK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use 'rm -f' with quotes here as well to safely remove the symlink (e.g., rm -f "$SYMLINK").
| rm $SYMLINK | |
| rm -f "$SYMLINK" |
|
Hi there, it's Minh again 👋 Thank you once again for your continued contributions. This is great. I’ll test it locally first, and in the meantime, would you mind helping us by consolidating the changes into a single PR that makes everything work seamlessly end-to-end? It’ll really help streamline testing and review. We appreciate your effort in making Jan better 🚀 |
|
Closing in favor of #5732 |
Part of #5641
Describe Your Changes
#5463 introduced a fix into the release workflow which exists outside of the standard make/yarn build process. This entailed upgrading linuxdeploy in /.cache/tauri. This is fine in a devcontainer/github runner, but manipulating the system cache on dev machines is bad form. To address this, a wrapper script is used to inject a new location for XDG_CACHE_HOME (moved to /.cache) in the build process as well as pin linuxdeploy. This PR is part of a series (#5641) which aims to pull essential build steps out of the release workflow so builds are reproducible on local dev machines, improving the dev experience of reviewing/contributing to Jan.
Self Checklist
Important
Fixes
EGL_BAD_PARAMETERbug by moving fix to local build process and using a wrapper script to manage cache andlinuxdeployversion.EGL_BAD_PARAMETERfix from GitHub release workflow to localmake/yarnbuild process.shim-linuxdeploy.shto setXDG_CACHE_HOMEto<project root>/.cacheand pinlinuxdeployversion.Makefileto remove./.cacheduring clean on Linux.package.jsonto useshim-linuxdeploy.shinbuild:tauri:linuxscript.linuxdeployversion pinning fromtemplate-tauri-build-linux-x64.yml.This description was created by
for a384721. You can customize this summary. It will automatically update as commits are pushed.