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

Improve linux building #111

Merged
merged 13 commits into from Dec 24, 2021
Merged

Improve linux building #111

merged 13 commits into from Dec 24, 2021

Conversation

kowalski7cc
Copy link
Contributor

This merge adds the following changes:
Move Linux related files in folder Platform/Linux
Change SpaceCadetPinball.desktop to reflect the original game name and description
Add a metainfo.xml file to support Appstream data
Add a new upscaled icon (at 192x192) that will be nicer in the launcher
Update readme.md with a brief description of the original game, and some instruction for building on Linux

@k4zmu2a
Copy link
Owner

k4zmu2a commented Dec 23, 2021

I don’t like some of these changes:
In .destop:
Keep existing Name=Space Cadet Pinball.
Comment – I assume the comment is about application, not the desktop file. Keep existing comment.

No need to change the readme, especially project name.
Readme space is precious, more so at the top. All non-essential info should be at the very bottom or skipped.
I consider all but audio issue to be non-essential.
Audio issue has wording problems, it implies that music does not work without steps shown.

The rest is good to go.

If you really want to, put your readme in a separate PR, where we will deconstruct it more thoroughly.

@kowalski7cc
Copy link
Contributor Author

For the .desktop file I was trying to mimic the Windows shortcut:
image

@k4zmu2a
Copy link
Owner

k4zmu2a commented Dec 23, 2021

Mimicking something is fine, but I think having just “pinball” as a name is misleading.
I rather have specific project name used there.

@kowalski7cc
Copy link
Contributor Author

@k4zmu2a I only left a couple of lines for who doesn't know what Space Cadet was. Also changed in desktop files to the name of the project

@k4zmu2a
Copy link
Owner

k4zmu2a commented Dec 23, 2021

It is better now, but still does not fully satisfy my arbitrary standards :)

I want the old .desktop Comment back.
Maxis never published 3DBP, they had Full Tilt! Pinball. Also remove the newline.
Do not format source ports table.
Readme should not have specific commands, rather general advice. Remove the install/build section.

@kowalski7cc
Copy link
Contributor Author

kowalski7cc commented Dec 23, 2021

Readme should not have specific commands, rather general advice. Remove the install/build section.

I do not fully agree. Especially people that approaches Linux usually doesn't know how to build from sources, so I added an example.

@kowalski7cc
Copy link
Contributor Author

Other things changed

@NicolaSmaniotto
Copy link
Contributor

If you want to create a Linux specific folder, we could move the .png icons there as well.

Also, for what is worth, I agree with @k4zmu2a about the commands in the readme. Compiling and installing from source (from a random github page) is not something an inexperienced user should do. A nicer approach would be to create and maintain packages in the correct repositories.

@kowalski7cc
Copy link
Contributor Author

@NicolaSmaniotto I think other platforms could benefit from the icons.

For the readme, I've seen many with the commands, or at least with the correct name of the libs for a couple of platforms. Like Debian and Fedora tend to have very different naming for some libraries. An experienced user can just ignore it.

@NicolaSmaniotto
Copy link
Contributor

I don't mean to sound rude or insistent, but I can't find a way to word this better:

correct name of the libs for a couple of platforms

That's only interesting to distro maintainers. Users don't need that info, not in the readme.

Debian and Fedora tend to have very different naming for some libraries

And it's the job of the maintainers to know the correct name for (just) their own.

An experienced user can just ignore it.

In my mind they are the only ones who would try to compile it themselves.

I still believe that users should not be encouraged to install software without the package manager, as this can have pretty serious repercussions. This extra information about how to compile (and troubleshoot audio) is probably better suited for a dedicated BUILD.md file, maybe in the Linux specific folder.

@kowalski7cc
Copy link
Contributor Author

I like the BUILD.md idea.

@kowalski7cc
Copy link
Contributor Author

I still believe that users should not be encouraged to install software without the package manager

Nor install packages maintained from unknown third parties... (I wonder how many people reads the PKGBUILD from AUR before installing). At that point probably building from GitHub doesn't sound so bad.
Also, the current readme encourages Linux users to build themself the binary, and the provided packages are already for distributions used by experienced people.

@k4zmu2a k4zmu2a merged commit 4db4e5f into k4zmu2a:master Dec 24, 2021
@k4zmu2a
Copy link
Owner

k4zmu2a commented Dec 24, 2021

Merged, that you for your patience.

For obscure programs compiling from source is often the only universal way to run them.
To facilitate this, I tried to make the build as simple as possible.
Flatpak will be the first universal alternative, but that is not for everyone.

Separate in depth docs are fine with me, I already have some in /Doc.

@k4zmu2a
Copy link
Owner

k4zmu2a commented Dec 24, 2021

One more thing, why would you use ninja generator in BUILD.md?
I think default makefile generator would make the build simpler.

@kowalski7cc
Copy link
Contributor Author

It was suggested on the flathub submission that ninja builds faster. I don't know if it's placebo, but it seems faster on my i3

@kowalski7cc kowalski7cc deleted the improve-linux branch December 24, 2021 11:01
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.

None yet

3 participants