SLiM icon indicating copy to clipboard operation
SLiM copied to clipboard

Uncomment the version macro in cmake call

Open bryce-carson opened this issue 2 years ago • 10 comments

Yep, my comment was a foreshadowing of build errors on COPR. Ha! Uh-oh!

bryce-carson avatar Sep 27 '22 20:09 bryce-carson

Worse, is the changes made now complicate the RPM building. I need to hold on the release of the new packaging style for openSUSE because the RPM macro I'm using is not defined in openSUSE, so I need to keep them on 4.0.1-1 (and can change only Fedora based distros to the new CMake stuff now).

bryce-carson avatar Sep 27 '22 20:09 bryce-carson

Oh, I was wrong. https://en.opensuse.org/openSUSE:Build_system_recipes#cmake

openSUSE does support these macros, but there was an issue with the build directory. I'll need to ask the friendly folks in the openSUSE #packaing discord. 🦎

bryce-carson avatar Sep 27 '22 20:09 bryce-carson

Hmm. Well, as usual, let me know when you want me to merge. If you're regretting the previous PR merge, you could always back those changes out for a while. :->

bhaller avatar Sep 27 '22 20:09 bhaller

Hmm. Well, as usual, let me know when you want me to merge. If you're regretting the previous PR merge, you could always back those changes out for a while. :->

It should be okay for now. The changes are acceptable, the hiccup was not expected from the documentation. I may have misread it. For now, the openSUSE users won't be affected, their system will continue to use the previous build of the RPM (no software changes) automatically.

Fedora users won't be negatively impacted either since dnf will also, automatically, only use a successful build from COPR. 👍🏼

I'm converting this to a draft until I have the fix for the openSUSE macro. Then you only need to merge one PR, instead of two.

bryce-carson avatar Sep 27 '22 20:09 bryce-carson

OK, groovy. I never noticed there was such a thing as a "draft" PR; where in GitHub's user interface did you set that?

bhaller avatar Sep 27 '22 21:09 bhaller

It is shown, when not already a Draft PR, in this listing on the right of the 'conversation'. It's easy to miss! Screenshot from 2022-09-27 15-14-21

bryce-carson avatar Sep 27 '22 21:09 bryce-carson

https://github.blog/2019-02-14-introducing-draft-pull-requests/

You can open a PR as a draft, and it cannot be merged until removed from draft state, using the above.

You can also find a small, gray hyperlink in the right-side listing to convert an open issue to a draft.

bryce-carson avatar Sep 27 '22 21:09 bryce-carson

Aha, good to know! Maybe I've just never noticed it before! :->

bhaller avatar Sep 27 '22 21:09 bhaller

FYI, I've just been doing some edits in CMakeLists.txt so you might get a conflict there, sorry!

bhaller avatar Sep 28 '22 16:09 bhaller

FYI, I've just been doing some edits in CMakeLists.txt so you might get a conflict there, sorry!

No worries. It'll probably be fine, and the merge won't be too hard I imagine.

bryce-carson avatar Sep 28 '22 17:09 bryce-carson

Hi @bryce-carson. Just a warning shot: I'll be releasing SLiM 4.1 pretty soon now. If this PR represents something that needs to happen, then it should happen ASAP; let me know. And I hope you'll be available to do the release stuff...? Thanks!

bhaller avatar Nov 14 '23 18:11 bhaller

Hi @bryce-carson. Just a warning shot: I'll be releasing SLiM 4.1 pretty soon now. If this PR represents something that needs to happen, then it should happen ASAP; let me know. And I hope you'll be available to do the release stuff...? Thanks!

Hi Ben!

Yeah, I should have availability to turn the gears on the RPM.

The Flatpak end, which this PR was made for a long while ago, is stalled because I need to ask am expert how to allow the application to start and own other processes transparently, without any regard to the build process of the application (I don't want any C++ changes).

RPM will be doable, though.

bryce-carson avatar Nov 19 '23 21:11 bryce-carson

Hi @bryce-carson. Just a warning shot: I'll be releasing SLiM 4.1 pretty soon now. If this PR represents something that needs to happen, then it should happen ASAP; let me know. And I hope you'll be available to do the release stuff...? Thanks!

Hi Ben!

Yeah, I should have availability to turn the gears on the RPM.

Hi @bryce-carson. OK, great. Stay tuned. :->

The Flatpak end, which this PR was made for a long while ago, is stalled because I need to ask an expert how to allow the application to start and own other processes transparently, without any regard to the build process of the application (I don't want any C++ changes).

Huh. I guess this has been stalled for a long time, right? Shall I just remove the section in the manual about installing using Flatpak? It's confusing for users to present an installation method in the manual that does not, in fact, work; and it seems to be problematic. I vote that either we remove all mention of the Flatpak installer, or we (meaning you, sorry :->) get it working again for 4.1. I am presently aiming to release 4.1 soon after Thanksgiving – end of November, or (if necessary due to some delay somewhere) early December. What do you think?

bhaller avatar Nov 20 '23 14:11 bhaller

Agreed!

On Mon., Nov. 20, 2023, 7:27 a.m. Ben Haller, @.***> wrote:

Hi @bryce-carson https://github.com/bryce-carson. Just a warning shot: I'll be releasing SLiM 4.1 pretty soon now. If this PR represents something that needs to happen, then it should happen ASAP; let me know. And I hope you'll be available to do the release stuff...? Thanks!

Hi Ben!

Yeah, I should have availability to turn the gears on the RPM.

Hi @bryce-carson https://github.com/bryce-carson. OK, great. Stay tuned. :->

The Flatpak end, which this PR was made for a long while ago, is stalled because I need to ask an expert how to allow the application to start and own other processes transparently, without any regard to the build process of the application (I don't want any C++ changes).

Huh. I guess this has been stalled for a long time, right? Shall I just remove the section in the manual about installing using Flatpak? It's confusing for users to present an installation method in the manual that does not, in fact, work; and it seems to be problematic. I vote that either we remove all mention of the Flatpak installer, or we (meaning you, sorry :->) get it working again for 4.1. I am presently aiming to release 4.1 soon after Thanksgiving – end of November, or (if necessary due to some delay somewhere) early December. What do you think?

— Reply to this email directly, view it on GitHub https://github.com/MesserLab/SLiM/pull/358#issuecomment-1819167489, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMMRAZKRSWHJR33NQTWXICDYFNSDLAVCNFSM6AAAAAAQXETXS2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJZGE3DONBYHE . You are receiving this because you were mentioned.Message ID: @.***>

bryce-carson avatar Nov 20 '23 15:11 bryce-carson

OK, I'll remove Flatpak from the manual for the time being; we can always add it back in later. Since this issue is Flatpak-related, I'm going to mark in "long-term" now, since it will not make the 4.1 release.

bhaller avatar Nov 20 '23 15:11 bhaller

Sounds good, thanks!

On Mon., Nov. 20, 2023, 8:37 a.m. Ben Haller, @.***> wrote:

OK, I'll remove Flatpak from the manual for the time being; we can always add it back in later. Since this issue is Flatpak-related, I'm going to mark in "long-term" now, since it will not make the 4.1 release.

— Reply to this email directly, view it on GitHub https://github.com/MesserLab/SLiM/pull/358#issuecomment-1819301824, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMMRAZJK4TEX75GCKBLURADYFN2K5AVCNFSM6AAAAAAQXETXS2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJZGMYDCOBSGQ . You are receiving this because you were mentioned.Message ID: @.***>

bryce-carson avatar Nov 20 '23 20:11 bryce-carson

Hi @bryce-carson. Just to make sure we're on the same page, what's the motivation for closing this issue altogether?

bhaller avatar Nov 21 '23 19:11 bhaller

Hi @bryce-carson. Just to make sure we're on the same page, what's the motivation for closing this issue altogether?

Hi Ben,

I closed this PR because it represents a misstep in refactoring the RPM spec file. The previous iteration of the spec file (the RPM package version)⸺4.0.1-1⸺worked for every RPM-based distribution I targeted, and all of the changes I made to it kept breaking something on at least one distribution.

Looking back at the commit history of this PR, the build completion status (success/failure) on COPR, and the goal that I had in mind with it (simplify the RPM spec file using macros as requested by the Flathub engineers), it was a misstep in hindsight.

While Flatpak would provide a prettier cross-distribution release method, it was too many headaches. Looking at the sandboxing (which in our case needs to be bypassed using Flatpak's approved means) again, it was a big drawback.

I believe, if we want a more or less one-or-two-click installation pathway for Linux user's that aren't supported by the RPM packages I will continue to maintain that AppImage is the distribution format we should've chosen all that time ago.

We already distribute binary packages to the community ourselves, and AppImage could even be integrated into the CI on GitHub (on every build, every tag, or every release; that'd be up to you if you want that).

I'm arguing now for using AppImage.

I'm going to make a test, and it should "just run" everywhere, really. AppImage is widely supported, as is Flatpak, but it is markedly easier for the packager, especially for our use-case (an application which requires no sand-boxing).

bryce-carson avatar Nov 21 '23 20:11 bryce-carson

Hi @bryce-carson. Just to make sure we're on the same page, what's the motivation for closing this issue altogether?

It would also help to know, given the documentation I've read so far, if Qt5 automatically leverages GPU-hardware acceleration in QtSLiM. I can include all dependencies in the AppImage, but it causes some issues for Nvidia GPU users.

bryce-carson avatar Nov 21 '23 20:11 bryce-carson

I'm arguing now for using AppImage.

OK @bryce-carson, as always I am happy to trust you on these matters; I understand very little of the issues involved, and I can't even use Linux any more on my home machines since VirtualBox seems to have stopped working for me (sigh). So, if you think AppImage is the way to go, I'm good with that. It sounds like you're looking into making that happen now; thanks! Please let me know how the chapter 2 doc will need to be modified; I can send you a snip of chapter 2 in Pages whenever you're ready to edit it, if you like, that has worked well in the past. Do you want to open a new issue about shifting to AppImage, as a better spot for continuing this discussion?

It would also help to know, given the documentation I've read so far, if Qt5 automatically leverages GPU-hardware acceleration in QtSLiM. I can include all dependencies in the AppImage, but it causes some issues for Nvidia GPU users.

Well, QtSLiM explicitly and directly uses OpenGL to obtain hardware acceleration for specific views inside the window – the individuals view and the chromosome view. I believe they do receive hardware acceleration as intended; if they didn't the drawing would be glacial. :-> Other views are drawn using Qt's Painter class; I believe the back end for that is also OpenGL, but that is internal to Qt, and hardware acceleration may not be guaranteed. I'd be surprised if it isn't, though. I doubt you need to include any GPU-acceleration dependencies in the AppImage beyond what Qt itself brings in; I think Qt ought to be self-sufficient. Certainly a regular command-line CMake build of QtSLiM, including on Linux, does not require linking to any external dependencies beyond Qt and OpenGL (specifically, Qt5::Widgets Qt5::Core Qt5::Gui OpenGL::GL). I hope that answers your question!

What's your timeframe for this AppImage shift, do you think? I'm still looking at wanting to ship SLiM 4.1 in the vicinity of Dec. 1; there is some work yet to be done in unit testing and documentation, but I think Dec. 1 is perhaps realistic.

bhaller avatar Nov 21 '23 21:11 bhaller