Skip to content

Further attempt to fix qt5 build #1106

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

Merged
merged 11 commits into from
Jan 29, 2025
Merged

Further attempt to fix qt5 build #1106

merged 11 commits into from
Jan 29, 2025

Conversation

DarthGandalf
Copy link
Contributor

No description provided.

@DarthGandalf DarthGandalf marked this pull request as ready for review January 28, 2025 23:57
@DarthGandalf
Copy link
Contributor Author

Now this is ready to merge, as you can see by the green tick :)

@@ -62,6 +62,7 @@ jobs:


package-exe:
if: ${{ github.event_name == 'push' }}
Copy link
Member

Choose a reason for hiding this comment

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

For what do we need those guards?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the PR there's no access to secrets, therefore upload fails

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though it may be a good idea to move the guards closer to upload itself. Then it'll be possible to download the build artifact from the PR even without creating the release


if [[ "$QT_VERSION" == 6* ]]; then
echo "USE_QT6=yes" >> $GITHUB_ENV
Copy link
Member

Choose a reason for hiding this comment

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

I think we have used the convention true/false or on/off.

host: 'linux'
install-deps: 'true'

- name: Install dependencies
run: sudo apt-get install extra-cmake-modules libxcb-xfixes0-dev xvfb

- name: Install Qt6 dependencies
Copy link
Member

Choose a reason for hiding this comment

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

This one is going to be installed for Gt5 and Qt6, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going to add an "if" here, but then decided that it doesn't hurt qt5 either, and eventually qt5 build would be removed anyway...

So went with just a comment instead

@DamirPorobic DamirPorobic merged commit 1ab2004 into ksnip:master Jan 29, 2025
10 checks passed
@DamirPorobic
Copy link
Member

Let's see how it works :)

@DarthGandalf DarthGandalf deleted the qt5 branch January 29, 2025 08:22
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.

2 participants