Skip to content

Conversation

Kidev
Copy link

@Kidev Kidev commented May 5, 2025

Decouples the build of viewsvg from that of resvg

  • Builds of viewsvg are now described in their own CI file (viewsvg-release.yml), are manual, and target a specific release tag. This workflow builds viewsvg for Windows, Linux and macOS, and adds their binaries inside the release that was targeted. Vast majority of changes have been moved to that file. See my post below for details and more screenshots
    image

  • The changes outside of that new CI file are the following (detailed reasons here):

    • Update action actions/checkout to v4
    • Update runners to macos-15, windows-2022 and ubuntu-24.04
    • Make use of C++20 inside the file tools/viewsvg/viewsvg.pro file (Qt6 requires >= C++17)
    • Remove QApplication::setAttribute(Qt::AA_EnableHighDpiScaling) from tools/viewsvg/main.cpp (default behavior in Qt6)
    • Make variable capture explicit inside lambdas in tools/viewsvg/svgview.cpp

@Kidev Kidev changed the title Use Qt6 to build viewsvg Add generation of viewsvg for MacOS, Windows, Linux using Qt6 May 6, 2025
@LaurenzV
Copy link
Collaborator

LaurenzV commented May 6, 2025

I am honestly not super fond of having this in CI again given that me (and others) are not familiar with Qt and wouldn't be able to fix it in case it breaks again, but given that you've already put the time into this, in case it looks good to @RazrFalcon I guess we can merge as is.

@DJMcNab
Copy link
Member

DJMcNab commented May 6, 2025

Thanks for fixing this up! I'm sure the changes to get it building will be useful for some users.

I do however agree with Laurenz. I'd also strongly push back against having this as part of the release process. QT has shown itself to be fragile to build conditions, and I don't want to go through this same process again in a year.

I can see arguments for setting up an "advisory" job, which checks that this still builds. But we still don't really have the bandwidth for that.

But I agree - @RazrFalcon can make a decision here.

@RazrFalcon
Copy link
Collaborator

I appreciate the patch, but you have done too much. We do not need AppImage. We do not need C++20. We do not need an app icon. Etc.
We just want a quick fix for a CI. And a fix that is easy to maintain.

Since I'm no longer maintaining this project, it's for others to decide. If others find it complicated, then we're closing it.

The whole point behind viewsvg is to test Qt C API wrapper and have a GUI demo for resvg rendering. viewsvg basically an ad.
But since resvg is already popular enough, we can probably deprecate viewsvg completely.

@LaurenzV
Copy link
Collaborator

LaurenzV commented May 6, 2025

I think I'm fine merging this if we can reduce this to the absolute minimum (basically, the same we have before in CI but actually working). But yes, please strip away all the other stuff that was added additionally.

@Kidev
Copy link
Author

Kidev commented May 6, 2025

That’s exactly why I prefer to comment or offer guidance instead of patches on CI in repos I’m less familiar with, modifying the CI code often feels like getting involved in someone else's family matters 😁

However, I didn't add much, here's my rationale for the changes


  • Make use of C++20 and Qt6 to build the binary viewsvg
    ↳ For Qt6, this is the recommended version. The minimum required is C++17

  • Add back the generation of the viewsvg binary for MacOS and Windows tagged releases
    ↳ I used the history of the file tagged-release.yml to add back only what you removed because it did not work with Qt5. I used explicitly qmake6 instead of just qmake which would have worked as well, to make it very clear that this is now dealing with Qt6

  • Add the generation of the viewsvg binary (AppImage) for Linux tagged releases (and the app icon)
    ↳ Maybe the most controversial I guess, it looks a bit verbose but the process is very similar to MacOS or Windows, except Qt does not provide a linuxdeploy binary, so I used this popular, nice and maintained tool. My OCD wanted to fix that 2/3 major OS supported. For the app icon, I had to add one to create a .AppImage and since there was nothing already in place, I just created a very simple svg for it dynamically.

  • Remove the now useless and deprecated QApplication::setAttribute(Qt::AA_EnableHighDpiScaling)
    ↳ This simply removes a function that has no effect in Qt6 as it is now the default behavior. Leaving it throws a warning at compile time

  • Make variable capture explicit inside lambdas
    ↳ It always was bad practice, but it is as of C++20 deprecated to implicitly capture this, as it is a very common source of annoying lifetime issues. Leaving it throws a warning at compile time. Here in reality, the issue is deeper: this is not at all the right way to do this using Qt. The correct approach is to leverage Qt signals, which inherently avoid issues related to this's lifetime or thread errors, at no extra cost. But it would require a little more changes (here is a quick patch that would be the safe Qt6 way to achieve what the code tries to achieve with a QTimer)

    -    // Run method in the m_worker thread scope.
    -    QTimer::singleShot(1, m_worker, [this, s](){
    -        m_worker->render(s);
    -    });
    +    // Emit signal to request rendering in worker thread
    +    emit renderRequested(s);
  • Update actions
    ↳ The old versions you were using are unsupported, and for example, install-qt-action prior to v4.2.1 is unlikely to work anymore because Github entirely removed action/cache@v3 and below

  • Update runners
    ↳ Used sensible versions that are the long-term good idea:

    • MacOS for Xcode15
    • Windows because MSVC2019 is a month away from deprecation
    • Linux because using latest is the best way to create trouble for your future selves when Github changes its meaning overnight and breaks your CI

What are your concerns exactly? I can change anything if you want I don't mind, but I really think that except for the AppImage I did the minimum here

@DJMcNab
Copy link
Member

DJMcNab commented May 6, 2025

Add back the generation of the viewsvg binary for MacOS and Windows tagged releases
↳ I used the history of the file tagged-release.yml to add back only what you removed because it did not work with Qt5. I used explicitly qmake6 instead of just qmake which would have worked as well, to make it very clear that this is now dealing with Qt6

As we have stated both in this thread and elsewhere, we didn't remove this because it didn't work with qt5, we removed it because we don't have the expertise to meaningfully maintain C++ code or a build which has proven itself to be fragile. And so having it in the repository is fine, but blocking releases working properly on it not fine.

My suggested way forward would be to revert the changes in ci.yml. That is, only have the changes inside the tools/viewsvg folder. Bumping the macOS, linux and windows runners (plus actions/checkout) is worthwhile, although should probably be a different PR. I don't expect you to make that PR.

I want to be clear about what's going wrong here - I really do appreciate that you're willing to update this. But I'm not going to ask you to keep this maintained, and therefore we need to minimise how much maintenance is needed.

@Kidev
Copy link
Author

Kidev commented May 6, 2025

As we have stated both in this thread and elsewhere, we didn't remove this because it didn't work with qt5, we removed it because we don't have the expertise to meaningfully maintain C++ code or a build which has proven itself to be fragile. And so having it in the repository is fine, but blocking releases working properly on it not fine.

Oh... well I missed that. I was under the assumption that a CI issue with installing Qt made you remove a feature, something I wanted to fix given that I contribute a lot to aqtinstall. Honestly I discovered this repo because it referenced a pinned closed issue on aqtinstall, that was misleading people because pinned issues always appear on top even when closed (I wrote the whole official qt installer feature for aqtinstall and install-qt-action so I was sad a weird Github feature made people miss it). When I saw the issue post #902, I mainly saw this:

Upgrade to Qt 6

  • This issue is fixed in Qt 6.
  • ⚠️ Significant migration effort required.

I use Qt daily and the repo looked very simple and was using features untouched by the migration, so I figure I could help since for me I knew it would be easy (having experience with doing so on large 3D projects). Did not read all that much more, so my knowledge of your project is more bottom-up than the opposite

My suggested way forward would be to revert the changes in ci.yml. That is, only have the changes inside the tools/viewsvg folder. Bumping the macOS, linux and windows runners (plus actions/checkout) is worthwhile, although should probably be a different PR. I don't expect you to make that PR.

I can move all that is viewsvg related in its own CI file, that is triggered only manually (workflow_dispatch). This action could get the latest release of resvg, build viewsvg against it, and append the binaries to the existing release. I could do that fairly easily

I want to be clear about what's going wrong here - I really do appreciate that you're willing to update this. But I'm not going to ask you to keep this maintained, and therefore we need to minimise how much maintenance is needed.

Don't worry I 100% get it, and this is the right decision for a project. I am not aware of the issues you had in the past, and I don't want to create some. In fact if I contribute to projects it's for the exact opposite reason 😄

I would just add that viewsvg is not, with my experience, a project that would take very long to maintain at all, and I'm not against maintaining a tool like that on the side really, beleive it or not I do that for fun :shipit: I could even write a quick fork that could be integrated with git-submodules to decouple it even more from the Rust part of the source, which is where all the added value of your project is

@thekingofcity
Copy link

Hi, I would really appreciate your work as a resvg mac user. Sorry if the following misleads you. I just did some investigation on the build failure on mac and hope to help fix it. I'm not familiar with Qt either, nor am I a maintainer in the repo, so the following may not reflect the view from the collaborators.

  1. Upgrade to Qt 6
    • This issue is fixed in Qt 6.
    • ⚠️ Significant migration effort required.

@Kidev
Copy link
Author

Kidev commented May 7, 2025

No worries! I literally crashed into a closed issue and threw a patch, I can't then just stand there and complain that I misread a thread that I barely read 😆

@Kidev
Copy link
Author

Kidev commented May 7, 2025

So, here's how it works: the build of viewsvg is longer automatic, ever.
To build it, a maintainer must go into Actions > ViewSVG Release > Run workflow. Here you can write a tag against which you'd like to build viewsvg with and for (it will checkout the code from the release with the tag you give it, use the version of resvg of that release, and the viewsvg source of that release). You can leave it empty and it'll take the last release. It'll then build viewsvg for each OS, add the binaries to the existing targeted release, and if at least one OS built it well, it'll add the line 'viewsvg' is a simple application that showcases resvg capabilities to the top of the release. If ran multiple times, the most recent binaries will overwrite the older ones (viewsvg only of course, it does not touch anything else). Here's screenshots:

To run it using and on the latest release
2025-05-07_19-54

To run it on v0.46.0 and using v0.46.0
2025-05-07_20-25

2025-05-07_19-56

The result! Pay attention to the updated time
2025-05-07_20-29

@Kidev Kidev changed the title Add generation of viewsvg for MacOS, Windows, Linux using Qt6 Decouple the build of viewsvg from that of resvg May 7, 2025
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