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-util/pyfa: version bump to 1.37.0 #7367

Closed

Conversation

ZeroPointEnergy
Copy link
Contributor

Package-Manager: Portage-2.3.19, Repoman-2.3.6

Simple version bump, no changes to the ebuild

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull Request assignment

Areas affected: ebuilds
Packages affected: games-util/pyfa

games-util/pyfa: a.zuber[at]gmx.ch, @gentoo/proxy-maint

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 ping us to reset the assignment.

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.

@gentoo-repo-qa-bot gentoo-repo-qa-bot added assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits. labels Mar 5, 2018

eapply_user

touch __init__.py
Copy link
Member

Choose a reason for hiding this comment

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

??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intention of the application developers is that pyfa is installed somewhere in the users home directory. The ebuild however installs it in the proper locations for python packages and since there is no __init__.py coming with the sources one is created to make python recognize it as a proper package.

The ebuild and program works without this, but I think it is the right thing to do here: https://docs.python.org/3/tutorial/modules.html#packages

Copy link
Member

Choose a reason for hiding this comment

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

Just ||die, please.

Copy link
Member

Choose a reason for hiding this comment

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

I'd also suggest adding a comment so that future maintainers aren't confused with this line.


pyfa_make_configforced() {
mkdir -p "${BUILD_DIR}" || die
sed -e "s:%%SITEDIR%%:$(python_get_sitedir):" \
Copy link
Member

Choose a reason for hiding this comment

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

missing || die

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

sed -e "s:%%SITEDIR%%:$(python_get_sitedir):" \
-e "s:%%EPREFIX%%:${EPREFIX}:" \
"${FILESDIR}/configforced-1.15.1.py" > "${BUILD_DIR}/configforced.py"
sed -e "s:%%SITEDIR%%:$(python_get_sitedir):" \
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

Copy link
Member

@mgorny mgorny left a comment

Choose a reason for hiding this comment

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

Please squash the changes afterwards.

inherit git-r3
KEYWORDS=""
else
SRC_URI="https://github.com/pyfa-org/Pyfa/archive/v${PV}.tar.gz -> pyfa-${PV}.tar.gz"
Copy link
Member

Choose a reason for hiding this comment

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

I think pyfa-${PV} is equivalent to ${P}, isn't it?


eapply_user

touch __init__.py
Copy link
Member

Choose a reason for hiding this comment

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

Just ||die, please.


src_install() {
pyfa_py_install() {
local packagedir=$(python_get_sitedir)/${PN}
Copy link
Member

Choose a reason for hiding this comment

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

Please use python_domodule to install to site-packages. It should handle any kind of data files too.

python_foreach_impl pyfa_py_install

insinto /usr/share/${PN}
doins eve.db
Copy link
Member

Choose a reason for hiding this comment

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

I hope this file isn't modified at runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an asset database for the program. The program does not change it at runtime, it only gets updated with new releases.

popd > /dev/null || die

dodoc README.md
doicon -s 32 imgs/gui/pyfa.png
Copy link
Member

Choose a reason for hiding this comment

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

This is desktop.eclass now, not eutils.

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 added the desktop eclass, but the eutils is still needed because of a call to edos2unix

}

pkg_preinst() {
gnome2_icon_savelist
Copy link
Member

Choose a reason for hiding this comment

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

This is no longer necessary.

Copy link
Member

Choose a reason for hiding this comment

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

I was talking about savelist, not update calls. Updates are still necessary/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, sorry

@ZeroPointEnergy
Copy link
Contributor Author

Implemented the requested changes and squashed the commits onto the current master. I also added yet another release which came out two days ago.

PYTHON_COMPAT=( python2_7 )
PYTHON_REQ_USE="sqlite,threads"

inherit eutils desktop python-r1 python-utils-r1
Copy link
Member

Choose a reason for hiding this comment

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

Please sort lexically whenever possible, and don't inherit python-utils-r1 directly.

DESCRIPTION="Python Fitting Assistant - a ship fitting application for EVE Online"
HOMEPAGE="https://github.com/pyfa-org/Pyfa"

LICENSE="GPL-3+ LGPL-2.1+ CC-BY-2.5 free-noncomm"
Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry if you've been asked about this already but do you actually need all those licenses simultaneously?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are absolutely right, I don't think they are correct. The software itself is GPL-3, but there are game assets bundled with the code which have another licenses and it is not clear to me what they are. I opened a ticked about it upstream and try to investigate this. May take some time.

Copy link
Member

Choose a reason for hiding this comment

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

If those assets are installed and are indeed licensed with all those licenses, then LICENSE is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, sorry for the long delay. The developer just got an answer from the asset owners and it's more or less "we don't know". pyfa-org/Pyfa#1505

What he proposes is that we include the text snipped he has on the bottom of the README here: https://github.com/pyfa-org/Pyfa/blob/master/README.md

So should I just add that text as a licence file and add that that as license together with GPL3 for the pyfa code? Is there a naming convention for this (I looked around but found none)?

CCP-assets-pfya (because the text is granting the rights to pyfa specifically) ?

Copy link
Member

Choose a reason for hiding this comment

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

Let's wait for @gentoo/licenses to advise. In the meantime, please implement all the remaining remarks.

Copy link
Member

Choose a reason for hiding this comment

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

I have already commented at pyfa-org/Pyfa#1505 but I must admit that I am somewhat at a loss concerning the license situation.

Especially, is the text below "License" at https://community.eveonline.com/community/fansites/toolkit/ a statement from the assets' copyright holder? To whom is it addressed (i.e., who is "you" in that text)? Note that the only clause that grants any rights is "we will give you a license to use them". All the rest of that text are conditions imposed on "you" restricting that usage right. There is no clause that would grant the right to redistribute.

My current conclusion would be that LICENSE needs to include "all-rights-reserved" (because it certainly isn't "free-noncomm") along with RESTRICT="mirror bindist".

I would also suggest that you file a Gentoo bug (with licenses team in CC or as assignee) if you want to further discuss the licensing situation, so that we have a traceable record of any decisions taken.

pyfa_py_install() {
python_moduleinto ${PN}
python_domodule eos gui service utils config*.py __init__.py
[[ -e info.py ]] && python_domodule info.py # only in zip releases
Copy link
Member

Choose a reason for hiding this comment

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

Are '.zip releases' even relevant here? Please change the condition to express when the file is expected, so that it'd explicitly fail when the file unexpectedly is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file does not exist anymore even in the zip archives it seems. I will remove the line

@@ -0,0 +1,88 @@
# Copyright 1999-2018 Gentoo Foundation
Copy link
Member

Choose a reason for hiding this comment

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

Please make it one commit per version.

@ZeroPointEnergy
Copy link
Contributor Author

Implemented the remaining remarks, apart from the license issue

@FuzzyGophers
Copy link
Contributor

@ZeroPointEnergy still want to work on this?

@ZeroPointEnergy
Copy link
Contributor Author

@flappyports Yes, I implemented all the requested changes but the situation with the license seems to be the issue why this is not moving on: pyfa-org/Pyfa#1505

Can we merge the new ebuild while the license stuff gets sorted? Old versions of the program get useless really fast for the users as the game parameters change.

@ZeroPointEnergy ZeroPointEnergy changed the title games-util/pyfa: version bump to 1.35.2 games-util/pyfa: version bump to 1.37.0 May 29, 2018
@ZeroPointEnergy
Copy link
Contributor Author

@ulm @mgorny @monsieurp Ok, I just rebased and updated the ebuild to the newest version with all the requested changes and finally also with all the license changes. Sorry this took so long, there where a lot of personal things which came suddenly up and I just had no free time to work on this until now.

The live ebuild does not work any-more, I updated the licence information and restrictions anyway in a separate commit. The reason why it currently breaks is that pyfa has moved to wxpython 4, which is missing in gentoo and will block any further updates of this software for now.

Package-Manager: Portage-2.3.24, Repoman-2.3.6
Package-Manager: Portage-2.3.24, Repoman-2.3.6
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2018-05-30 12:56 UTC
Newest commit scanned: 740c75d
Status: ✅ good

No issues found

@mgorny
Copy link
Member

mgorny commented May 30, 2018

Thanks!

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
6 participants