Remove requirement for separate Qt installation #677
Conversation
WalkthroughThe changes involve updates across various configuration files to enhance the Python environment setup, optimize package installation processes, and clarify dependency management. Key updates include transitioning to Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- .github/actions/init-environment/action.yml (1 hunks)
- .github/workflows/build-multi-os.yml (14 hunks)
- README.md (4 hunks)
- setup-environment.ps1 (1 hunks)
- src/main/resources/base/ayab/translations/ayab_trans.pl (1 hunks)
Files skipped from review due to trivial changes (3)
- .github/actions/init-environment/action.yml
- README.md
- setup-environment.ps1
Additional comments not posted (26)
src/main/resources/base/ayab/translations/ayab_trans.pl (2)
90-90: Comment update is accurate.The comment now correctly reflects the usage of
pyside6-lreleaseinstead oflrelease.
92-92: Update topyside6-lreleaseis correct.The update from
lreleasetopyside6-lreleaseis correct and aligns with the PR objectives.Ensure that the
pyside6-lreleasecommand works as intended and produces the expected.qmfiles..github/workflows/build-multi-os.yml (24)
30-30: Update toactions/checkout@v4is correct.The update from
actions/checkout@v3to@v4ensures compatibility with the latest features and fixes.
51-51: Update toactions/cache/save@v4is correct.The update from
actions/cache/save@v3to@v4ensures compatibility with the latest features and fixes.
57-57: Usingapt-getis appropriate.Switching to
sudo apt-get updateavoids warnings and is more suitable for scripts.
66-66: Update toactions/cache@v4is correct.The update from
actions/cache@v3to@v4ensures compatibility with the latest features and fixes.
72-72: Update toactions/cache@v4is correct.The update from
actions/cache@v3to@v4ensures compatibility with the latest features and fixes.
78-78: Update toactions/cache@v4is correct.The update from
actions/cache@v3to@v4ensures compatibility with the latest features and fixes.
84-84: Update toactions/cache@v4is correct.The update from
actions/cache@v3to@v4ensures compatibility with the latest features and fixes.
90-90: Update toactions/cache@v4is correct.The update from
actions/cache@v3to@v4ensures compatibility with the latest features and fixes.
96-96: Update toactions/cache@v4is correct.The update from
actions/cache@v3to@v4ensures compatibility with the latest features and fixes.
110-110: Update toactions/cache/save@v4is correct.The update from
actions/cache/save@v3to@v4ensures compatibility with the latest features and fixes.
116-116: Update toactions/cache/save@v4is correct.The update from
actions/cache/save@v3to@v4ensures compatibility with the latest features and fixes.
122-122: Update toactions/cache/save@v4is correct.The update from
actions/cache/save@v3to@v4ensures compatibility with the latest features and fixes.
128-128: Update toactions/cache/save@v4is correct.The update from
actions/cache/save@v3to@v4ensures compatibility with the latest features and fixes.
134-134: Update toactions/cache/save@v4is correct.The update from
actions/cache/save@v3to@v4ensures compatibility with the latest features and fixes.
140-140: Update toactions/cache/save@v4is correct.The update from
actions/cache/save@v3to@v4ensures compatibility with the latest features and fixes.
150-150: Update toactions/checkout@v4is correct.The update from
actions/checkout@v3to@v4ensures compatibility with the latest features and fixes.
177-177: Update toactions/checkout@v4is correct.The update from
actions/checkout@v3to@v4ensures compatibility with the latest features and fixes.
193-193: Update toactions/cache/restore@v4is correct.The update from
actions/cache/restore@v3to@v4ensures compatibility with the latest features and fixes.
201-201: Update toactions/cache/restore@v4is correct.The update from
actions/cache/restore@v3to@v4ensures compatibility with the latest features and fixes.
209-209: Update toactions/cache/restore@v4is correct.The update from
actions/cache/restore@v3to@v4ensures compatibility with the latest features and fixes.
217-217: Update toactions/cache/restore@v4is correct.The update from
actions/cache/restore@v3to@v4ensures compatibility with the latest features and fixes.
225-225: Update toactions/cache/restore@v4is correct.The update from
actions/cache/restore@v3to@v4ensures compatibility with the latest features and fixes.
233-233: Update toactions/cache/restore@v4is correct.The update from
actions/cache/restore@v3to@v4ensures compatibility with the latest features and fixes.
107-107: Ensuresetup-environment.ps1runs correctly.The script
setup-environment.ps1is used to convert UI and translation files.Ensure the script runs correctly and performs the intended actions.
|
Since the Build workflow doesn’t trigger for PRs here’s a run on my fork on the same commit, to demonstrate everything works: https://github.com/jonathanperret/ayab-desktop/actions/runs/9908222560 |
There was a problem hiding this comment.
Can you break the changes out into multiple PRs? I'm ok removing QT, but I am less prepared for testing the updates to actions at the moment, and I added the type generation for a reason so I would rather we don't remove it without further discussions and testing.
jonathanperret
force-pushed
the
no-standalone-qt-required
branch
from
2f47e49 to
fd0b2f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- .github/workflows/build-multi-os.yml (2 hunks)
- README.md (4 hunks)
- src/main/resources/base/ayab/translations/ayab_trans.pl (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- .github/workflows/build-multi-os.yml
- README.md
- src/main/resources/base/ayab/translations/ayab_trans.pl
You're right, I got a bit carried away there. I've pared down the PR to just the removal of Qt. As I added in the PR description, on Linux we need to somehow get Qt's system dependencies installed, so I left |
|
@X-sam as @jonathanperret did some changes to the PR, do you think we can merge it now? |
jonathanperret
force-pushed
the
no-standalone-qt-required
branch
from
fd0b2f3 to
d1e3066
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- .github/workflows/build-multi-os.yml (2 hunks)
- README.md (4 hunks)
- src/main/resources/base/ayab/translations/ayab_trans.pl (1 hunks)
Files skipped from review due to trivial changes (1)
- README.md
Additional comments not posted (4)
src/main/resources/base/ayab/translations/ayab_trans.pl (2)
90-90: LGTM!The comment accurately reflects the change to use
pyside6-lrelease.
92-92: LGTM!The command change to
pyside6-lreleaseensures compatibility with PySide6..github/workflows/build-multi-os.yml (2)
107-107: LGTM!The comment clarifies the purpose of installing Qt6 packages, improving documentation for future maintainers.
108-108: LGTM!The command ensures that necessary system dependencies are installed without requiring a separate Qt installation.
The only reason to require a standalone Qt installation was the `lrelease` tool used to convert the translation files. It turns out PySide6, which we install through `pip`, already includes this tool under the name `pyside6-lrelease`. This changes the call from `lrelease` to `pyside6-lrelease`, and removes the installation of Qt from the GitHub Actions workflow and from the development setup instructions, except on Linux where we install Qt tools just to bring in the required system libraries.
jonathanperret
force-pushed
the
no-standalone-qt-required
branch
from
d1e3066 to
b235841
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- .github/workflows/build-multi-os.yml (2 hunks)
- README.md (4 hunks)
- src/main/resources/base/ayab/translations/ayab_trans.pl (1 hunks)
Files skipped from review due to trivial changes (1)
- src/main/resources/base/ayab/translations/ayab_trans.pl
Files skipped from review as they are similar to previous changes (2)
- .github/workflows/build-multi-os.yml
- README.md
The only reason to require a standalone Qt installation was the
lreleasetool used to convert the translation files.It turns out PySide6, which we install through
pip, already includes this tool under the namepyside6-lrelease.This PR changes the call from
lreleasetopyside6-lrelease, and removes the installation of Qt from the GitHub Actions workflow and from the development setup instructions.Except on Linux, where the Qt libraries which are bundled in PySide6 have themselves dependencies on system libraries such as
libgl1and a dozen others. In that case it seemed simpler to keep installing a system Qt 6 package to pull these system dependencies in, even though none of the Qt components installed system-wide will be used.Summary by CodeRabbit
New Features
Documentation
Chores