-
Notifications
You must be signed in to change notification settings - Fork 273
Decouple the build of viewsvg from that of resvg #917
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
base: main
Are you sure you want to change the base?
Conversation
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. |
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. |
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. 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 |
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. |
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
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 |
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 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. |
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
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
I can move all that is
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 |
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.
|
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 😆 |
Decouples the build of
viewsvg
from that ofresvg
Builds of

viewsvg
are now described in their own CI file (viewsvg-release.yml
), are manual, and target a specific release tag. This workflow buildsviewsvg
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 screenshotsThe changes outside of that new CI file are the following (detailed reasons here):
actions/checkout
tov4
macos-15
,windows-2022
andubuntu-24.04
tools/viewsvg/viewsvg.pro
file (Qt6 requires >= C++17)QApplication::setAttribute(Qt::AA_EnableHighDpiScaling)
fromtools/viewsvg/main.cpp
(default behavior in Qt6)tools/viewsvg/svgview.cpp