Skip to content
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

(Linux/macOS) Various installation suggestions #3

Closed
OPNA2608 opened this issue Oct 12, 2020 · 11 comments
Closed

(Linux/macOS) Various installation suggestions #3

OPNA2608 opened this issue Oct 12, 2020 · 11 comments

Comments

@OPNA2608
Copy link
Contributor

This applies to Linux in particular, but the other major OS' would benefit from it too: Please make the installation prefix user-defined, hard-coding them is never a good idea.

ptcollab/src/editor.pro

Lines 177 to 180 in cb670e6

# Default rules for deployment.
qnx: target.path = /tmp/$${TARGET}/bin
else: unix:!android: target.path = /opt/$${TARGET}/bin
!isEmpty(target.path): INSTALLS += target

The user / their build system manager might want to install them somewhere else, mine for example expects the project file to handle the quasi-standard PREFIX to designate what should be considered the root directory to place /bin, /lib, /share/[projectname] etc under. This path can be very different from /opt; in my case, PREFIX=/nix/store/ccbbas68vlr0d52h8wda9l30rma9gzn8-ptcollab-0.3.4 will be passed to the make invocation. A default should of course still be supplied, which can be /opt or /usr.

For an example how to handle this nicely, you can check out BambooTracker's project file.

https://github.com/rerrahkr/BambooTracker/blob/9fd422431e99f6bd76ad79db7fe1b0e67cf54430/BambooTracker/BambooTracker.pro#L25-L39

https://github.com/rerrahkr/BambooTracker/blob/9fd422431e99f6bd76ad79db7fe1b0e67cf54430/BambooTracker/BambooTracker.pro#L602-L627

@yuxshao
Copy link
Owner

yuxshao commented Oct 13, 2020

Hey, thanks for pointing this out! I actually hadn't paid any attention to those lines before.

Poking around a bit it seems like they were added by qtcreator by default, which is a bit surprising because I agree it looks nonstandard. I'll take a look and fix it up.

@yuxshao
Copy link
Owner

yuxshao commented Oct 13, 2020

Okay, I think this should be good! Thanks for the helpful examples too. Let me know if there are any other problems.

@OPNA2608
Copy link
Contributor Author

Hope you don't mind if I escalate this issue abit and make it about various small install-time things I'm noticing while packaging ptcollab for a Linux/macOS package manager, making an issue for each of them feels abit excessive.

I commented something on your commit that needs to be fixed for the application icon to be installed properly. If you don't plan to add other fixed resolutions of that icon though, then you can simplify it abit.

Where's the icon from, is there a higher-quality source? The freedesktop's icon theme specifications permit multiple nominal resolutions for better-quality bitmap icons at higher resolutions and optional SVG for smooth scalability, hence the resolution-looping code in BambooTracker. (an SVG made no sense for us because the original icon is pixel art)
If you have an SVG source (or I can trace your bitmap icon) then you / I could add that to the install process - the target path for installing would be $$PREFIX/share/icons/scalable/apps/ptcollab.svg

I saw that the ZIPs on the release page have sample instruments and songs, however in the repository I can only find the zipped-up instruments - that also don't have install targets. You should definitely add all of those to the repo (preferably in unzipped form to cut down on dependencies) and install them to $$PREFIX/share/ptcollab/…. For the instrument taken straight from pxtone, creating a pxtone subdirectory to cleanly separate them from (maybe in the future?) user-submitted instruments and songs would seem like the sensible approach to me.

Lastly, the version information is currently broken for me.

grafik

This is due to the project file extrapolating the version information via git & the .git repository directory at build time.

ptcollab/src/editor.pro

Lines 16 to 17 in 7d99c13

GIT_VERSION = $$system(git --git-dir $$PWD/../.git --work-tree $$PWD describe --tags)
DEFINES += GIT_VERSION=\"\\\"$$GIT_VERSION\\\"\"

For reproducibility reasons, the build manager I'm using throws away the .git directory before starting the build. Build inputs, including the source code, are verified with a hash to ensure as much reproducibility as possible, and the .git directory is sadly a source of indeterminism due to various actions being able to change its contents despite fetching a pinned version of the code. Would it be possible to replace this mechanism with a simple header file that contains the current release version, however finely-grained you need it to be? Our example from BambooTracker here.

@OPNA2608 OPNA2608 changed the title Make installation path user-defined (Linux/macOS) Various installation suggestions Oct 13, 2020
@yuxshao
Copy link
Owner

yuxshao commented Oct 13, 2020

Ah, got it. Thanks so much for this! Here I was thinking using the git version was clever, haha.

There is an icon.svg in the src directory - it's unused in the code but was what generated the mac icons. I added the sample songs to the zip after realizing that mistake too. It's still in a zip, but I'll get to fixing that and the other issues sometimes today (unless you'd be willing to take a stab?).

@OPNA2608
Copy link
Contributor Author

Ah damn, I had already spent some time nicely recreating the icon in Inkscape. 😄

(unless you'd be willing to take a stab?)

I'm abit busy with tasks, but I'll see what I can do. Could take awhile though, so you're prolly faster.

@yuxshao
Copy link
Owner

yuxshao commented Oct 13, 2020

Haha, your version may well be crisper than the existing icon. And that sounds good, don't feel any pressure - thanks again!

@yuxshao
Copy link
Owner

yuxshao commented Oct 14, 2020

All right, this should be done! Assuming I didn't misconfigure anything else...

@OPNA2608
Copy link
Contributor Author

The 16px icon seems too small (has transparent space on some sides instead of fitting to the border) & off-center.

grafik

Aside from this, it builds, installs and works fine (offline at least, haven't tried any online kerfuffle yet). Thanks!

Haha, your version may well be crisper than the existing icon.

Well it's a vector graphic, it can't get any crisper 😜

@yuxshao
Copy link
Owner

yuxshao commented Oct 14, 2020

Ah, great! Glad all of that works, thanks again!

As for the small icon, it's borrowed from an older project as a favicon and was set to 15x15 so the triangle could be centred in the dark box and not have to stretch. It didn't seem distracting in the browser to me, but maybe it's distracting in another desktop view? (or maybe it's always been distracting and I've just gotten numb to it haha)

@OPNA2608
Copy link
Contributor Author

Sorry for the late response.

The smaller icon is not a deal breaker, but I can never unsee it now xp. It just looks unintentional I guess.

I tested it online and everything worked as well, very nice. One of my friends on macOS was bummed because she's on 10.13 High Sierra but the build manually sets the lowest supported version to 10.14 Mojave. Out of curiosity, is there something that needs this bumped target to fix? The default for Qt 5.15 is 10.13.

QMAKE_MACOSX_DEPLOYMENT_TARGET = 10.14

I consider the issue solved in any case, many thanks!

@yuxshao
Copy link
Owner

yuxshao commented Oct 20, 2020

Great, glad it worked for the most part! Thanks again for helping with this.

I'll revisit why it's 10.14, but my vague memory is that when it wasn't set, there a number of errors for C++17 features. But I will double check and try to fix that.

Edit: Yeah, unfortunately it has to do with the fact that std::variant and std::optional (which are used pretty heavily in the source) are not available 10.14 or earlier :(

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

No branches or pull requests

2 participants