games-rpg/{draci-historie,drascula,dreamweb,gwiz,soltys}: EAPI8, ebuild improvements #21464
Conversation
Pull Request assignmentSubmitter: @mm1ke games-rpg/draci-historie: @gentoo/games Linked bugsNo bugs to link found. If your pull request references any of the Gentoo bug reports, please add appropriate GLEP 66 tags to the commit message and request reassignment. If you do not receive any reply to this pull request, please open or link a bug to attract the attention of maintainers. In order to force reassignment and/or bug reference scan, please append Docs: Code of Conduct ● Copyright policy (expl.) ● Devmanual ● GitHub PRs ● Proxy-maint guide |
gentoo-bot
added
assigned
Yes, this wall of copy/paste is looking pretty worthless. It's okay if not, but if you're interested in trying to improve these it'd be welcome. Have a tendency to semi-rewrite most of those old game ebuilds when I touch them myself, some of them were barely improved since 2003 and their original maintainer is gone -- sometimes even broken by drive-by / games.eclass migration too.
Early adoption may not always be the best thing to do, but we're talking about old ~arch-only game ebuilds here that may not be touched again for a long time, I don't see a reason not to start using 8 wherever the eclasses allow it (using l10n.eclass would notably mean no eapi8 yet, but that's fine). I notably started doing this in my last small batch of commits anyhow, not that many devs jumped into this yet (Edit: there's only a few perl ebuilds, app-doc/pms, one intel ebuild, and few games ebuild I changed that use EAPI=8 right now, first eapi8 ebuild I pushed was this because I wanted fetch+ and dosym8). |
|
|
||
| if use l10n_en || ( ! use l10n_cs && ! use l10n_de && ! use l10n_en && ! use l10n_pl ); then | ||
| make_wrapper draci-historie-en "scummvm -f -p \"${EPREFIX}/usr/share/${PN}/en\" draci" . | ||
| make_desktop_entry ${PN}-en "Dračí Historie (English)" "${EPREFIX}/usr/share/pixmaps/draci-historie.ico" |
There was a problem hiding this comment.
Not a full review but this bothers me. Nice to have thought of prefix but this whole thing was wrong to start with, not only should desktop entries not have full path to icons, but icons other than png, xpm, and svg are all invalid as far as XDG is concerned and won't be displayed in most cases.
Optimally the idea would be to convert them and add them to SRC_URI but I've personally been lazy with these and just use the generic "applications-games" icon (i.e. make_desktop_entry name Name applications-games) for now and remove bad icons. Going through all of these and eventually giving them proper icons in my devspace is a TODO I'm keeping for later (for old games anyway).
There was a problem hiding this comment.
Actually, if any icons are bad / missing I'll fix these on merge now (feel free to leave a applications-games placeholder) -- don't want to get too backlogged on this so started adding them to devspace now.
There was a problem hiding this comment.
Thanks for doing that. As you suggested i've now simply use the applications-games placeholder (only for draci-historie now, but going to update the others too) :)
There was a problem hiding this comment.
You can add the icon now if you want (similar to the old .ico):
https://dev.gentoo.org/~ionen/distfiles/${PN}.png (draci-historie.png)
doicon "${DISTDIR}"/${PN}.png
And update Manifest.
For make_desktop_entry, remove the applications-games placeholder and it'll use ${PN}.png by default.
In this batch, was there any other missing/wrong? I see xpm and png on others, and that sounds okay (assuming they are square, I've had a case where the icon was 32x33 and it wouldn't display 👀).
There was a problem hiding this comment.
I've checked all icons and the others seems to be fine except for games-rpg/eschalon-book-1-demo (which doesn't seems to have a icon). I guess using applications-games would be the right way to fix it (or getting a icon for this game) but since it doesn't even start (see comment below) i haven't touched the ebuild for now.
Pull request CI reportReport generated at: 2021-06-29 09:24 UTC There are existing issues already. Please look into the report to make sure none of them affect the packages in question: |
Pull request CI reportReport generated at: 2021-07-02 19:59 UTC There are existing issues already. Please look into the report to make sure none of them affect the packages in question: |
I've now looked into Mainly I didn't like that because for two reason's:
However, my other solution works similar like the function
In order to make it even more simple i've also added One more thing to note: I've added a I've also checked the installed files with the original ebuild and afaics it's the same except for the I think this is a better solutions as with EDIT: Nevermind, my solution is pretty much the same as in |
Pull request CI reportReport generated at: 2021-07-02 21:19 UTC There are existing issues already. Please look into the report to make sure none of them affect the packages in question: |
|
Haven't reviewed the changes yet, but just to reply.
Sorry for making you waste time on checking this, as I mentioned I didn't have a close look at it myself yet. Thought we did have an eclass for generic l10n_ handling, but looking closer every ebuild is doing their own thing -- so it makes sense too here then. And, indeed I didn't mean to use LINGUAS at all here, so there's no need to convince me.
Using REQUIRED_USE sounds good to me, but yes, l10n_en should be default given not every users set L10N and then they won't be able to emerge the package without setting it (annoyance for them). There's no profile default for L10N (all disabled) unlike there is for PYTHON_TARGETS and the like. |
Although using REQUIRED_USE does have some disadvantages, making it default is nice but if users set say.. L10N="nl" it'll disable the default and now you have no language if _nl isn't provided as well, forcing them to add "en". |
No Problem. :) |
Hmm, you're right. It's a pity that the default setting with I'll update my PR tomorrow, it's already late, but thanks again for the hints. |
|
|
||
| LICENSE="GPL-2" | ||
| SLOT="0" | ||
| KEYWORDS="~amd64 ~x86" | ||
| IUSE="l10n_cs l10n_de l10n_en l10n_pl" | ||
| REQUIRED_USE="|| ( l10n_cs l10n_de l10n_en l10n_pl )" | ||
|
|
||
| RDEPEND=">=games-engines/scummvm-1.1" |
There was a problem hiding this comment.
Not everyone may agree but I'd argue should drop those >= checks when we reach 6+ years of being gone from the tree (1.1 was added in 2010 and <1.1 was removed at same time), or at least on non-core packages.
| local loc | ||
| for loc in cs de en pl; do | ||
| if use l10n_${loc}; then |
There was a problem hiding this comment.
Admit I do like the simplicity with REQUIRED_USE and I wouldn't have minded keeping it, but if want it to be nicer for users than it is for us (which is certainly not a bad thing).
There's many ways to go about it (like a function, or alternate styling, if anything you prefer), but for one option that keeps it simple (don't want to overdo this):
| local loc | |
| for loc in cs de en pl; do | |
| if use l10n_${loc}; then | |
| MY_L10N=( $(usev l10n_cs) $(usev l10n_de) $(usev l10n_en) $(usev l10n_pl) ) | |
| [[ ${MY_L10N} ]] || MY_L10N=( l10n_en ) | |
| local loc | |
| for loc in "${MY_L10N[@]//l10n_/}"; do |
Then you can re-use the non-local MY_L10N in src_install. Whether it's enabled is already checked so no need to run "if use" anymore. Also skips the need to repeat the locales an extra time.
Will still need to restore that ugly thing though:
!l10n_cs? ( !l10n_de? ( !l10n_en? ( !l10n_pl? ( ${BASE_URL}-en-${PV}.zip ) ) ) )
| " | ||
| S="${WORKDIR}" | ||
|
|
||
| LICENSE="GPL-2" | ||
| SLOT="0" | ||
| KEYWORDS="~amd64 ~x86" | ||
| IUSE="l10n_cs l10n_de l10n_en l10n_pl" |
There was a problem hiding this comment.
Maybe still set +l10n_en as default just to indicate it's used. REQUIRED_USE below can of course be dropped if changing method.
| src_prepare() { | ||
| default | ||
| rm -f *.{bat,exe,ins} readme.* || die | ||
| rm -f *.{bat,exe,ins,ico} readme.* || die | ||
| } |
There was a problem hiding this comment.
Hm, I was going to mention removing the -f, but I don't see the use for this src_prepare. Only the ${loc}/ directories are installed, and cleaning those seem pointless.
| local lingua | ||
| for lingua in $(find * -type d); do | ||
| doins -r ${lingua} | ||
| done |
There was a problem hiding this comment.
Since we have our own loop, just remove this block and add a doins -r ${loc} in the new one.
| make_desktop_entry ${PN}-${loc} "Dračí Historie (${loc})" applications-games | ||
| fi | ||
| done | ||
|
|
||
| einstalldocs |
There was a problem hiding this comment.
This einstalldocs isn't installing anything, you can drop it.
The readme.txt in the .zip doesn't seem to really apply to using this with our packaging and is kinda messy (src_prepare was deleting it), I don't think it's worth trying to preserve it.
Pull request CI reportReport generated at: 2021-07-04 08:54 UTC There are existing issues already. Please look into the report to make sure none of them affect the packages in question: |
|
@ionenwks
|
mm1ke
changed the title
mm1ke
force-pushed
the
game-tCfIc8tI
branch
2 times, most recently
from
3fd3f4f
to
c23789a
Compare
|
One more note. |
Pull request CI reportReport generated at: 2021-07-04 11:14 UTC There are existing issues already. Please look into the report to make sure none of them affect the packages in question: |
Pull request CI reportReport generated at: 2021-07-04 11:24 UTC There are existing issues already. Please look into the report to make sure none of them affect the packages in question: |
yep, adding [vorbis] also warrant a revbump on its own, dynamic deps are unreliable and a revbump will ensure the requirement is known on users' systems
More of a grey area here, I don't typically consider a .desktop icon relevant enough for a revbump, opinions may differ.
https://www.gog.com/forum/eschalon_series/linux_crash_on_startup/post5 Someone attempted a fix but it needs updating and is very fragile, and alternative is no good. Sounds like it'd be easy for upstream to fix but doesn't look like they've really done anything in years. If someone really want to play this I'd suggest they use Wine at this point. I'll make a note of it and throw it in a last-rite batch later. Don't waste more time with it. |
mm1ke
changed the title
Done, I've split the commit to reflect this (revbump + removal). Keeping the old is probably not required, especially since it has no stable keywords.
Ok, i'll keep it then :)
Agreed and removed from the PR |
Pull request CI reportReport generated at: 2021-07-04 13:59 UTC There are existing issues already. Please look into the report to make sure none of them affect the packages in question: |
| https://dev.gentoo.org/~ionen/distfiles/${PN}.png | ||
| l10n_cs? ( ${BASE_URL}-cz-${PV}.zip ) | ||
| l10n_de? ( ${BASE_URL}-de-${PV}.zip ) | ||
| l10n_en? ( ${BASE_URL}-en-${PV}.zip ) | ||
| l10n_pl? ( ${BASE_URL}-pl-${PV}.zip ) | ||
| !l10n_cs? ( !l10n_de? ( !l10n_en? ( !l10n_pl? ( ${BASE_URL}-en-${PV}.zip ) ) ) ) |
There was a problem hiding this comment.
It's just my own rule for sorting SRC_URI (which differs from DEPEND), but I prefer the main package to be listed first and support files (aka the icon) after, and these l10n_* happen to be the main package. I typically always put the icon dead bottom.
| insinto /usr/share/${PN} | ||
| for lingua in $(find * -type d); do | ||
| doins -r ${lingua} | ||
| doicon "${DISTDIR}"/${PN}.png |
There was a problem hiding this comment.
Having a doicon after a insinto feels a bit awkward, maybe move the doicon below the loop to keep the relation with the doins.
| src_prepare() { | ||
| default | ||
| rm -f *.{bat,exe,ins} readme.* || die | ||
| MY_L10N=( $(usev l10n_cs) $(usev l10n_de) $(usev l10n_en) $(usev l10n_pl) ) |
There was a problem hiding this comment.
Just noticed the l10n_cs .zip is named -cz-
| MY_L10N=( $(usev l10n_cs) $(usev l10n_de) $(usev l10n_en) $(usev l10n_pl) ) | |
| MY_L10N=( $(usev l10n_{cs,cz}) $(usev l10n_de) $(usev l10n_en) $(usev l10n_pl) ) |
Note that usev 2nd argument is a new EAPI-8 feature, in EAPI-7 we'd have to use:
$(usex l10n_{cs,cz} '')
|
|
||
| S="${WORKDIR}" | ||
| RDEPEND="games-engines/scummvm[vorbis]" | ||
| DEPEND="${RDEPEND}" |
There was a problem hiding this comment.
Drop DEPEND line, like the others scummvm is only needed at runtime.
| make_wrapper ${PN} "scummvm -f -p \"/usr/share/${PN}\" drascula" . | ||
| doicon "${DISTDIR}"/${PN}.png | ||
| make_wrapper ${PN} "scummvm -f -p \"${EPREFIX}/usr/share/${PN}\" drascula" . | ||
| make_desktop_entry ${PN} "Drascula: The Vampire Strikes Back" ${PN} |
There was a problem hiding this comment.
desktop eclass adds DESCRIPTION to the desktop entry automatically, so usually this is kept shorter
| make_desktop_entry ${PN} "Drascula: The Vampire Strikes Back" ${PN} | |
| make_desktop_entry ${PN} Drascula |
| } | ||
|
|
||
| src_prepare() { | ||
| default | ||
| rm -rf license.txt soltys-es-v1-0 | ||
| rm license.txt || die | ||
| } | ||
|
|
||
| src_install() { | ||
| insinto /usr/share/${PN} | ||
| doins -r * |
There was a problem hiding this comment.
Bit like the find, this doins -r * feels a bit too much.
So like the other ebuilds, add doins -r ${lang} in the loop, and move newicon after the loop.
| src_prepare() { | ||
| default | ||
| rm -rf license.txt soltys-es-v1-0 | ||
| rm license.txt || die | ||
| } |
There was a problem hiding this comment.
If no longer use doins -r *, this src_prepare also becomes useless like the others.
| for lang in "${MY_L10N[@]//l10n_/}"; do | ||
| mkdir -p ${lang} || die | ||
| unpack ${PN}-${lang}-v${PV}.zip | ||
| mv vol.{cat,dat} ${lang}/ || die |
There was a problem hiding this comment.
Unfortunately, the l10n_es zip comes ruin it by extracting in a subdir unlike the others:
>>> Unpacking soltys-es-v1.0.zip to /var/tmp/portage/games-rpg/soltys-1.0-r2/work
mv: cannot stat 'vol.cat': No such file or directory
mv: cannot stat 'vol.dat': No such file or directory
I don't really like doing it like this but well, I feel like splitting it again would be messier with the enable-english logic:
| mv vol.{cat,dat} ${lang}/ || die | |
| if [[ ${lang} == es ]]; then | |
| mv ${PN}-${lang}-v$(ver_rs 1 -)/vol.{cat,dat} ${lang}/ || die | |
| else | |
| mv vol.{cat,dat} ${lang}/ || die | |
| fi |
|
|
||
| local lang | ||
| for lang in "${MY_L10N[@]//l10n_/}"; do | ||
| mkdir -p "${S}"/${lang} || die |
There was a problem hiding this comment.
Drop the -p everywhere (including other ebuilds), it would be strange if "${S}" didn't already exist and creating leading directories was somehow needed.
| make_wrapper ${PN}-${lang} "scummvm -f -p \"${EPREFIX}/usr/share/${PN}/${lang}\" ${PN}" . | ||
| make_desktop_entry ${PN}-${lang} "Soltys (${lang})" ${PN} |
There was a problem hiding this comment.
The third argument on either of those isn't needed for any of these ebuilds.
.makes the wrapper change directory.. to the current dir, unspecifying does nothing which is current dir${PN}is the default for icon if unspecified
Package-Manager: Portage-3.0.20, Repoman-3.0.3 Signed-off-by: Michael Mair-Keimberger <mmk@levelnine.at>
Package-Manager: Portage-3.0.20, Repoman-3.0.3 Signed-off-by: Michael Mair-Keimberger <mmk@levelnine.at>
Package-Manager: Portage-3.0.20, Repoman-3.0.3 Signed-off-by: Michael Mair-Keimberger <mmk@levelnine.at>
Package-Manager: Portage-3.0.20, Repoman-3.0.3 Signed-off-by: Michael Mair-Keimberger <mmk@levelnine.at>
games-rpg/drascula was revbumped because of a missing dep on games-engines/scummvm[vorbis] and also because it requires a newer dat file to play. Package-Manager: Portage-3.0.20, Repoman-3.0.3 Signed-off-by: Michael Mair-Keimberger <mmk@levelnine.at>
Package-Manager: Portage-3.0.20, Repoman-3.0.3 Signed-off-by: Michael Mair-Keimberger <mmk@levelnine.at>
|
@ionenwks |
Pull request CI reportReport generated at: 2021-07-05 19:49 UTC There are existing issues already. Please look into the report to make sure none of them affect the packages in question: |
|
Merged as-is, thanks :) Much better than the old ebuilds. |
Hi,
This PR updates some
games-rpgebuilds. Some notes:if .. thenblocks to control the installation of the language. While I don't have any strong opinion about that i think it would look nicer and more compact to have it like in the ebuilddrasculato install them.EAPI=7but i see thatEAPI=8is getting more and more used so i think updating them straight toEAPI=8would be a good idea too? Any thoughts about this?@ionenwks @SoapGentoo