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

Importing distro files from Flatpak and Arch Linux #130

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

cengique
Copy link
Member

@cengique cengique commented Jun 2, 2024

Imported packaging files and new screenshots @AsciiWolf

misc/io.github.Tremulous.appdata.xml Outdated Show resolved Hide resolved
misc/io.github.Tremulous.appdata.xml Outdated Show resolved Hide resolved
misc/grangerhub-logo.png Outdated Show resolved Hide resolved
@AsciiWolf
Copy link

Looks much better now, thanks! :-)

misc/grangerhub-logo.png Outdated Show resolved Hide resolved
@cengique
Copy link
Member Author

cengique commented Jun 3, 2024

@AsciiWolf just pushed some more commits, please review. Thanks! :)

<screenshot><image>https://raw.githubusercontent.com/hughsie/fedora-appstream/master/screenshots-extra/tremulous/d.png</image></screenshot>
<screenshot><image>https://raw.githubusercontent.com/hughsie/fedora-appstream/master/screenshots-extra/tremulous/e.png</image></screenshot>
<screenshot type="default">
<image type="source" width="1280" height="960">https://github.com/GrangerHub/tremulous/blob/master/misc/grangerhub-trem1.3-7d1b.jpg?raw=true</image>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note: I may be wrong, but I think that the correct URL should be https://raw.githubusercontent.com/GrangerHub/tremulous/master/misc/grangerhub-trem1.3-7d1b.jpg.

Same with the other screenshots/videos.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this sounds right. I used to be able to get those URLs but somehow I can't seem to get to them. But just using your example works (for existing files). I will re-confirm once I merge this PR to master.

@AsciiWolf
Copy link

Nice, I think it looks good now (except for the screenshots URL)! The only remaining thing are the Makefile rules. :-)

@AsciiWolf
Copy link

AsciiWolf commented Jun 3, 2024

Although, we should probably sort through the screenshots, select the best ones and remove others. Currently, there are too many of them and some are very similar.

For example, misc/screenshots/c-fedora.png and misc/screenshots/d-fedora.png look almost the same.

@cengique
Copy link
Member Author

cengique commented Jun 4, 2024

Nice, I think it looks good now (except for the screenshots URL)! The only remaining thing are the Makefile rules. :-)

Great! I will sort through the screenshots, but I will need some help with the Makefile.

@wtfbbqhax @jkent can you help crafting an install section in the Makefile that can place binaries, icon, and desktop files in the proper places? As far as I see, we only have one target in the Makefile that just produces a zip or other file based on target distro. So we never worried about an install section. What's the situation with the CMake build? Was that ever working? Thanks!

@cengique
Copy link
Member Author

cengique commented Jun 4, 2024

Looks like libcurl is also outdated and causing Linux build errors due to USE_RESTCLIENT=1.

@cengique
Copy link
Member Author

@AsciiWolf @jkent I added an install: target in the Makefile. I'd appreciate if you can review. Note that the game client is not perfectly working in this branch, so I'll rebase to a release branch to complete testing. I'm looking for general feedback about the changes in the Makefile. Thanks!

@AsciiWolf I removed screenshot d-fedora.png, but kept the newer ones as not all are referenced in the flatpak.

Makefile Outdated
ifndef BASEGAME
BASEGAME=gpp
endif

BASEGAME_CFLAGS=-I../../${MOUNT_DIR}

ifndef COPYDIR
COPYDIR="/usr/local/games/tremulous"
COPYDIR="/opt/$(PACKAGE)"
Copy link

@AsciiWolf AsciiWolf Jun 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why /opt?

edit: Oh, the /opt is just a fallback for unknown platforms/configurations (without valid PREFIX)? If so, it makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was following what was done in the Arch Linux packaging. Since Tremulous binaries need to stay in the data folder, it doesn't correspond to the usual Linux way of storing stuff under PREFIX/share/APPNAME format. But I'm not confident in that. There used to be a separate tremulous-data package. Not sure which approach is better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, /usr/local/games seems to be an unused location in modern distros. Again, I'm not confident about this.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But /opt is usually not used anymore by most software. You can use something like $PREFIX/share/tremulous (/usr/local/share/tremulous if PREFIX is /usr/local).

Take a look how we package it in Fedora:
https://koji.fedoraproject.org/koji/rpminfo?fileOrder=name&rpmID=37498623&buildrootOrder=-id&buildrootStart=0#filelist
https://koji.fedoraproject.org/koji/rpminfo?fileOrder=name&rpmID=37497768&buildrootOrder=-id&buildrootStart=0#filelist

Makefile Outdated
# Install the .desktop, icon files, license, etc.
@$(INSTALL) -D -m644 "misc/io.github.grangerhub.Tremulous.png" "$(PREFIX)/share/pixmaps/tremulous.png"
@$(INSTALL) -D -m644 "misc/io.github.grangerhub.Tremulous.desktop" \
"$(PREFIX)/share/applications/tremulous.desktop"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is incorrect. The whole point of renaming the desktop file (and its matching launchable tag in the AppStream metadata file) was to keep it at the new io.github.grangerhub.Tremulous.desktop name. Installing it as tremulous.desktop makes no sense at this point and will break things for AppStream metadata.

If you want to install the desktop/icon files as just tremulous.*, rename the original files in the misc directory and also change the launchable tag in AppStream file to just "tremulous.desktop" (but don't change the actual id and metainfo file name - that should always stay in rDNS format).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I renamed them to Tremulous-Grangerhub.

Copy link

@AsciiWolf AsciiWolf Jun 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is still not ideal and even more broken now (since the actual icon file does not match Icon field in the desktop file). I was talking about the rDNS formatted name and I even sent suggestions to apply.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, rDNS naming conventions for desktop files will probably became compulsory on many Linux distributions / packaging formats in the future. So my change was also future-proof.

If you use this "Tremulous-Grangerhub" name format because of the upcoming official Grangerhub Snap, then please consider doing these changes in downstream (patch for the Snap package), not directly in upstream.

But it is up to you. I just do not personally like these changes, that's probably my problem.

Makefile Outdated
@$(INSTALL) -d $(PREFIX)/bin
@$(INSTALL) -D -m755 $(BR)/$(CLIENTBINSH) $(BR)/$(SERVERBINSH) $(PREFIX)/bin
# Install the .desktop, icon files, license, etc.
@$(INSTALL) -D -m644 "misc/io.github.grangerhub.Tremulous.png" "$(PREFIX)/share/pixmaps/tremulous.png"
Copy link

@AsciiWolf AsciiWolf Jun 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I understand. The icon name in the desktop file is io.github.grangerhub.Tremulous so it should be installed as io.github.grangerhub.Tremulous.png, not tremulous.png. If you want to install it as just tremulous.png, rename the icon file directly in the misc directory and also fix the "Icon" line in the desktop file to contain just "tremulous".

Also, please never use the pixmaps directory, it is deprecated and already was for last 15 years or so. Use $(PREFIX)/share/icons/hicolor/128x128/apps.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also renamed icon to Tremulous-Grangerhub. Corrected the install directory.

@AsciiWolf
Copy link

It looks good except the things that I pointed out. Thanks!

@AsciiWolf
Copy link

By the way, we have a CI build failure on Linux:

error: 'CURLMOPT_MAX_TOTAL_CONNECTIONS' conflicts with a previous declaration

Is it the same thing as already mentioned here?

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@cengique
Copy link
Member Author

By the way, we have a CI build failure on Linux:

error: 'CURLMOPT_MAX_TOTAL_CONNECTIONS' conflicts with a previous declaration

Is it the same thing as already mentioned here?

That's right, we will work on that separately, but for now you can build with:

USE_INTERNAL_LUA=1 make -j

by removing the building of the REST client.

@cengique
Copy link
Member Author

@AsciiWolf I think I addressed all the issues. Please check! Thanks :)

Makefile Outdated Show resolved Hide resolved
<launchable type="desktop-id">io.github.grangerhub.Tremulous.desktop</launchable>
<name>Tremulous</name>
<launchable type="desktop-id">Tremulous-Grangerhub.desktop</launchable>
<name>Tremulous-Grangerhub</name>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? This will only make it confusing for our users. You can instead mention in the description that this is a "Grangerhub version of Tremulous". And it will add me much work to do to adjust the Flatpak to this unnecessary change. :-/

[Desktop Entry]
Name=Tremulous
Comment=Aliens vs Humans, First Person Shooter game with elements of Real Time Strategy
Icon=io.github.grangerhub.Tremulous
Copy link

@AsciiWolf AsciiWolf Jun 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you really want to use the "Tremulous-Grangerhub" naming scheme (which I disagree with as I mentioned), you need to fix the Icon name here as well.

Makefile Outdated
install: release
ifneq (,$(findstring "$(PLATFORM)", "linux" "gnu_kfreebsd" "kfreebsd-gnu" "gnu"))
$(echo_cmd) "Installing for Linux platform in $(COPYBINDIR) and $(PREFIX)"
@$(INSTALL) -d $(PREFIX)/bin "$(PREFIX)/share/metainfo/" \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a very small nitpick here. I noticed that some paths have "/" at their end and some don't. It does not matter, but it would still be nice to make it consistent.

@AsciiWolf
Copy link

AsciiWolf commented Jun 16, 2024

So, I have thought about the desktop/icon file name one more time and I come to conclusion that you can probably keep the name at tremulous-grangerhub if you want, but please change the name to a lower case one (tremulous-grangerhub.desktop / tremulous-grangerhub.png). Most desktop files that are in legacy non-rDNS name format have their whole name in lowercase, and it also looks better that way and is more consistent with the original (tremulous.desktop / tremulous.png) name. And don't forget to update the Icon entry in the desktop file to use the correct icon.

But please, consider changing the <name> tag in AppStream metadata from <name>Tremulous-Grangerhub</name> back to <name>Tremulous</name> and adding an information about the GrangerHub Tremulous version to a <description> instead.

Thanks!

@cengique
Copy link
Member Author

So, I have thought about the desktop/icon file name one more time and I come to conclusion that you can probably keep the name at tremulous-grangerhub if you want, but please change the name to a lower case one (tremulous-grangerhub.desktop / tremulous-grangerhub.png). Most desktop files that are in legacy non-rDNS name format have their whole name in lowercase, and it also looks better that way and is more consistent with the original (tremulous.desktop / tremulous.png) name. And don't forget to update the Icon entry in the desktop file to use the correct icon.

But please, consider changing the <name> tag in AppStream metadata from <name>Tremulous-Grangerhub</name> back to <name>Tremulous</name> and adding an information about the GrangerHub Tremulous version to a <description> instead.

Thanks!

Hey @AsciiWolf , thanks for the compromising solution. I was a bit taken aback and wanted a meeting since this was taking too long. We can still have a short meeting to find the best solutions and finalize this. I have no problem changing the name tag in AppStream back to Tremulous. The reason for branding was that there are a couple of different clients around (the other one being Tremfusion) - but I guess we can name that one separately if it comes to it. Also the GrangerHub versions may be buggy and people may want to install vanilla versions (if they can find working ones!). But the GrangerHub info can stay in the description and version fields for now. I will change the files to lowercase as I noticed that it looked awkward in those folders, which mostly contained lowercase tools. Although, I saw a couple of font file names, etc, that had mixed case. Anyways, I'll think about the naming scheme one more time and will go through the list of items you picked above. Thanks.

@AsciiWolf
Copy link

Thanks for your reply! Meeting could be a good idea, but I am afraid that my time schedule does not allow me more meetings in upcoming week or two. :-/

Regarding the branding name: Most Linux distributions do not package Tremfusion or vanilla Tremulous anymore, so I would not worry about that. :-)

@cengique
Copy link
Member Author

So, I have thought about the desktop/icon file name one more time and I come to conclusion that you can probably keep the name at tremulous-grangerhub if you want, but please change the name to a lower case one (tremulous-grangerhub.desktop / tremulous-grangerhub.png). Most desktop files that are in legacy non-rDNS name format have their whole name in lowercase, and it also looks better that way and is more consistent with the original (tremulous.desktop / tremulous.png) name. And don't forget to update the Icon entry in the desktop file to use the correct icon.

But please, consider changing the <name> tag in AppStream metadata from <name>Tremulous-Grangerhub</name> back to <name>Tremulous</name> and adding an information about the GrangerHub Tremulous version to a <description> instead.

@AsciiWolf I think I fixed these, but please take a look at my latest commit. Thanks and sorry for the long delay. Too much traveling and not enough time on the computer.

[Desktop Entry]
Name=Tremulous Grangerhub
Comment=Aliens vs Humans, First Person Shooter game with elements of Real Time Strategy (Grangerhub version)
Icon=tremulous-grangerhub

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can change the icon file name to io.github.grangerhub.Tremulous.png instead. Same with the desktop file name (io.github.grangerhub.Tremulous.desktop).

@@ -0,0 +1,9 @@
[Desktop Entry]
Name=Tremulous Grangerhub

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the " Grangerhub" suffix in name really necessary if we already have it in the Comment section (and also in the Summary section of AppStream metadata file)?

We could optionally modify the icon file and add something like "GH" text to the corner to make the branding more apparent. But I do not think that changing the name of the actual game is a good idea. (Especially considering that the Grangerhub version is de facto the only version of Tremulous that is still used nowadays.)

<provides>
<id>tremulous-grangerhub.desktop</id>
</provides>
<launchable type="desktop-id">Tremulous-Grangerhub.desktop</launchable>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When/if the desktop file name is changed, we have to change it here (and also in the provides section) as well. Also, please note that the file name is case sensitive, so the current entry is incorrect.

Makefile Outdated
$(INSTALL) -D -m755 $(BR)/$(CLIENTBINSH) $(PREFIX)/bin/tremulous-grangerhub
$(INSTALL) -D -m755 $(BR)/$(SERVERBINSH) $(PREFIX)/bin/tremded-grangerhub
$(INSTALL) -D -m644 "misc/Tremulous-Grangerhub.png" "$(PREFIX)/share/icons/hicolor/128x128/apps/"
$(INSTALL) -D -m644 "misc/Tremulous-Grangerhub.desktop" \
Copy link

@AsciiWolf AsciiWolf Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait... Why are we installing the files with different (upper case) name? It does not make sense.

Makefile Outdated
@cd $(BR) && for file in $(NAKED_TARGETS); do \
$(INSTALL) -D $$file $(COPYBINDIR)/$$file; \
done
$(INSTALL) -D -m755 $(BR)/$(CLIENTBINSH) $(PREFIX)/bin/tremulous-grangerhub

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are still installing the tremulous and tremded binaries with -grangerhub suffixes. Is this intended?

@AsciiWolf
Copy link

Thanks! I have provided some feedback.

<launchable type="desktop-id">Tremulous-Grangerhub.desktop</launchable>
<name>Tremulous</name>
<developer_name>Dark Legion Development and GrangerHub</developer_name>
<summary>Aliens vs Humans, First Person Shooter game with elements of Real Time Strategy (Grangerhub version)</summary>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question: Is the correct name Grangerhub or GrangerHub?

- remove grangerhub suffix from binaries
- add fully qualified domain names to icon and desktop
- fix forgotten lines in Makefile from last commit
- In Makefile, remove setting of variable B that causes recursive
processing delay. Instead create a second target for recursive Make
call.
- Change install folder from /opt to $PREFIX/share/
@cengique
Copy link
Member Author

cengique commented Oct 7, 2024

@AsciiWolf just got around to fixing these issues. If you can check the conversations and resolve, we may be close to being done! If this is all good, I'll rebase this branch to a working release and do more testing.

[Desktop Entry]
Name=Tremulous
Comment=Aliens vs Humans, First Person Shooter game with elements of Real Time Strategy (GrangerHub version)
Icon=io.github.grangerhub.Tremulous.png

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Icon=io.github.grangerhub.Tremulous.png
Icon=io.github.grangerhub.Tremulous

The desktop icon should not contain a file type suffix.

ifndef BASEGAME
BASEGAME=gpp
endif

BASEGAME_CFLAGS=-I../../${MOUNT_DIR}

ifndef PREFIX
PREFIX=/usr

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default prefix should probably rather be /usr/local.

Players can choose from 2 unique races, aliens and humans.
Players on both teams are able to build working structures in-game like an
RTS.
</p>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the GrangerHub branding was removed from the Tremulous Flatpak/desktop file name, we could instead mention it in the description. :-)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see that it is mentioned in the summary. Maybe we could provide more information what the Grangerhub version is in the description?

@AsciiWolf
Copy link

AsciiWolf commented Oct 9, 2024

Looks very good now (except the few things mentioned before), thanks! :-)

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.

2 participants