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

games-rpg/lure: EAPI8 bump, ebuild improvements #21765

Closed
wants to merge 1 commit into from

Conversation

mm1ke
Copy link
Contributor

@mm1ke mm1ke commented Jul 24, 2021

Package-Manager: Portage-3.0.20, Repoman-3.0.3
Signed-off-by: Michael Mair-Keimberger mmk@levelnine.at

@ionenwks

Some notes:

  • I've decided to not install the notes.txt file since i don't think it's relevant on linux (and makes it easier for the ebuild ;))
  • Initially i even had the ebuild without a src_prepare. However, in case of PROTECT.PDF the file would have been compressed which is why i had to rename it. (i guess it's required for doc files that extension are lowercase?) Anyway, I couldn't find any rules about the file extension need to be lowercase which is why i keep it for the other files.

@gentoo-bot
Copy link

Pull Request assignment

Submitter: @mm1ke
Areas affected: ebuilds
Packages affected: games-rpg/lure

games-rpg/lure: @gentoo/games

Linked bugs

No 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 [please reassign] to the pull request title.

Docs: Code of ConductCopyright policy (expl.) ● DevmanualGitHub PRsProxy-maint guide

@gentoo-bot gentoo-bot added assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits. labels Jul 24, 2021
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2021-07-24 18:04 UTC
Newest commit scanned: 316be1d
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/7e7f8c6ecc/output.html

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2021-07-24 19:29 UTC
Newest commit scanned: 7fcda66
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/7c329a132d/output.html

Copy link
Contributor

@ionenwks ionenwks left a comment

Choose a reason for hiding this comment

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

Have to say the original ebuild for this is disgusting. I'm treating this as an entirely new ebuild by you (not that I have much to say beside trivialities given it looks good).

Comment on lines 8 to 10
DAT_PV="0.13.1"
DESCRIPTION="Play as the young peasant named Diermot who has to overthrow an evil sorceress"
Copy link
Contributor

Choose a reason for hiding this comment

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

Trivial but I prefer a blank line before description delimiting the new block.


LICENSE="lure"
SLOT="0"
KEYWORDS="~amd64 ~x86"
LANGS_IUSE="l10n_en l10n_es l10n_de l10n_fr l10n_it"
IUSE=${LANGS_IUSE}
IUSE="l10n_de +l10n_en l10n_es l10n_fr l10n_it"
RESTRICT="mirror"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, seeing this "mirror" made me wonder if it should also have bindist, but reading this license I don't see anything that requires mirror restrictions. All it talks about is about how much we can redistribute this (at least, given license file is provided, which it is in the repos' licenses/ dir).

Unless you noticed something that go against that, just remove the line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've also checked the license but also couldn't find a reason for the RESTRICT=mirror - i've removed it.

local lang name
for lang in "${MY_L10N[@]//l10n_/}"; do
[[ "${lang}" == "en" ]] && name="${PN}" || name="${PN}-${lang}"
mv ${name}/PROTECT.{PDF,pdf} || die
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use newdoc oldname newname in src_install rather than change files in the source directory, should let you get rid of this src_prepare after all.

Alternatively, it's possible to use docompress -x ... to exclude files from compression, but seems more sane to have extensions that aren't all over the place case-wise.

local lang
local lang name
for lang in "${MY_L10N[@]//l10n_/}"; do
[[ "${lang}" == "en" ]] && name="${PN}" || name="${PN}-${lang}"
Copy link
Contributor

@ionenwks ionenwks Jul 26, 2021

Choose a reason for hiding this comment

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

No need for these "", ${PN} and ${lang} won't have spaces. Furthermore, bash tests (aka [[ ]] form) can handle variables with spaces without quoting, so that's never needed either way.

The quotes for MY_L10N are normal to expand the array properly though.

Package-Manager: Portage-3.0.20, Repoman-3.0.3
Signed-off-by: Michael Mair-Keimberger <mmk@levelnine.at>
@mm1ke
Copy link
Contributor Author

mm1ke commented Jul 27, 2021

@ionenwks
Thanks for review. The tip with newdoc is perfect. Now the ebuild only contains a src_install
I've also updated the my_l10n variable. Since it's only used in src_install now i made it local and changed the name to lowercase.

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2021-07-27 17:24 UTC
Newest commit scanned: c50f05d
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/3079161c78/output.html

Copy link
Contributor

@ionenwks ionenwks left a comment

Choose a reason for hiding this comment

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

I've also updated the my_l10n variable. Since it's only used in src_install now i made it local and changed the name to lowercase.

Nice thought :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits.
Projects
None yet
4 participants