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-action/minetest: version bump to 0.4.16 #3874

Closed
wants to merge 3 commits into from

Conversation

Anth0rx
Copy link

@Anth0rx Anth0rx commented Feb 7, 2017

Gentoo-Bug: 604424

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull Request assignment

Areas affected: ebuilds
Packages affected: games-action/minetest

games-action/minetest: @gentoo/proxy-maint (maintainer needed)

@gentoo-repo-qa-bot gentoo-repo-qa-bot added maintainer-needed There is at least one affected package with no maintainer. Review it if you can. assigned PR successfully assigned to the package maintainer(s). labels Feb 7, 2017
@gktrk gktrk requested a review from SoapGentoo February 7, 2017 19:47
EBUILD minetest-0.4.13.ebuild 3210 SHA256 4a6d4fbccec809b0bc52101ea6dff188d608fbc1da58cb36e68a6ee35e071bf6 SHA512 c500846bd9ccd91189b761e950c349a0bbec404a58eb47d5fcdc3775c99d874f2d05cb9f20bfa87c805e8edeae21f939a394f3240cbdbd250ff4d7628a95bb4a WHIRLPOOL a8a71e1c50e5a951c3fe963cca49436bee152c7c8443c10d350bc2b355e0ee14a6564d4838ff3bf65a66094d178f8599904418940de510e25869c09524b1bf58
EBUILD minetest-0.4.14.ebuild 3157 SHA256 ce63d9acd9e9393583aa7de9a4c0fb7575b98cd36ac9d9e9cb33cf466a90597a SHA512 514dcd11f1143bd96480de71c821c7eee7b7d1f993da6a0b02f6222d22d264042101831b0c18887eff6fc8c05eeaf856309b8398fa9d9b536349ecbd8658ffe3 WHIRLPOOL e3e4200ce34533e9192cbf11eecbbc73db08db1f6aba1ec80c472cea6ed336373cd948fd8610d4660d25af7e364559fce4f06a153592847e9e22fc0815543df0
EBUILD minetest-0.4.15.ebuild 3157 SHA256 ce63d9acd9e9393583aa7de9a4c0fb7575b98cd36ac9d9e9cb33cf466a90597a SHA512 514dcd11f1143bd96480de71c821c7eee7b7d1f993da6a0b02f6222d22d264042101831b0c18887eff6fc8c05eeaf856309b8398fa9d9b536349ecbd8658ffe3 WHIRLPOOL e3e4200ce34533e9192cbf11eecbbc73db08db1f6aba1ec80c472cea6ed336373cd948fd8610d4660d25af7e364559fce4f06a153592847e9e22fc0815543df0
MISC metadata.xml 1974 SHA256 d58cb792e3212a7cb79ae1b52651697f26908764aa2a966102030f937049bb15 SHA512 73f1aae76e6d363abe2cb5e87e7481850119334c6fc65e50760d7de9afb87a9b81fc66c907390c183d8ef98c9df381a77f825c89f29c9bf645d8ff9c7f46eb5f WHIRLPOOL 9a74006d37702fa27df0539c4cccb2c606ed6c38494ba62248fe65153b8867dedd3d30501ae9d427e0c9a8858ca577f320c193283d0d4ca3d75bd84fd50ebff3
Copy link
Member

Choose a reason for hiding this comment

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

same here, AUX, MISC, EBUILD need to be gone

@@ -0,0 +1,139 @@
# Copyright 1999-2016 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 use repoman full to check that the ebuild has no QA violations, this should be 2017

!dedicated? (
app-arch/bzip2
>=dev-games/irrlicht-1.8-r2
dev-libs/gmp:0
Copy link
Member

Choose a reason for hiding this comment

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

:0=

Copy link
Author

@Anth0rx Anth0rx Feb 9, 2017

Choose a reason for hiding this comment

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

Are the mentioned slot operators meant to be applied on all mentioned dependencies?

Copy link
Member

Choose a reason for hiding this comment

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

nope, just line preceding it (gmp in this case)

app-arch/bzip2
>=dev-games/irrlicht-1.8-r2
dev-libs/gmp:0
media-libs/libpng:0
Copy link
Member

Choose a reason for hiding this comment

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

:0=

sound? (
media-libs/libogg
media-libs/libvorbis
media-libs/openal
Copy link
Member

Choose a reason for hiding this comment

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

:=

}

src_prepare() {
eapply_user
Copy link
Member

Choose a reason for hiding this comment

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

chuck this, and do cmake-utils_src_prepare instead

src_configure() {
local mycmakeargs=(
-DBUILD_CLIENT=$(usex !dedicated)
-DCUSTOM_BINDIR="/usr/bin"
Copy link
Member

Choose a reason for hiding this comment

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

all these dirs should have ${EPREFIX} added, like so:
-DCUSTOM_BINDIR="${EPREFIX}/usr/bin"

)

use dedicated && mycmakeargs+=(
-DIRRLICHT_SOURCE_DIR=/the/irrlicht/source
Copy link
Member

Choose a reason for hiding this comment

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

is this even a proper directory?

Copy link
Author

Choose a reason for hiding this comment

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

Actually I just used the ebuild file of the previous version. Is there a way to test ebuild files properly (like in a sandboxed mode)?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you can cd into the package directory and use the ebuild command to invoke various phases. e.g. ebuild minetest-0.4.15.ebuild configure

Copy link

Choose a reason for hiding this comment

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

Could this be from the minetest wiki? Quote from Compiling Minetest#Building without Irrlicht / X dependency:

You can build the Minetest server without library dependencies to Irrlicht or any graphical stuff. You still need the Irrlicht headers for this, so first, download the irrlicht source to somewhere.
When invoking CMake, use -DBUILD_CLIENT=0 -DIRRLICHT_SOURCE_DIR=/wherever/you/unzipped/the/source
So probably even for just the dedicated server, irrlicht should be a build time dependency, then giving the directory manually should not be necessary.

Copy link
Member

Choose a reason for hiding this comment

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

thats the update here? Can you include the path to the irrlicht sources?

cmake-utils_src_compile

if use doc ; then
cmake-utils_src_compile doc
Copy link
Member

Choose a reason for hiding this comment

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

add after this:
HTML_DOCS=( "${CMAKE_BUILD_DIR}"/doc/html/. )
and then...

if use doc ; then
cd "${CMAKE_BUILD_DIR}"/doc || die
dodoc -r html
fi
Copy link
Member

Choose a reason for hiding this comment

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

... you can remove this entire if block

-DCUSTOM_BINDIR="${EPREFIX}/usr/bin"
-DCUSTOM_DOCDIR="${EPREFIX}/usr/share/doc/${PF}"
-DCUSTOM_LOCALEDIR="${EPREFIX}/usr/share/${PN}/locale"
-DCUSTOM_SHAREDIR="${EPREFIX}/usr/share/${PN}"
-DCUSTOM_EXAMPLE_CONF_DIR="/usr/share/doc/${PF}"
Copy link

@quazgar quazgar Feb 15, 2017

Choose a reason for hiding this comment

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

${EPREFIX} here as well for the CUSTOM_EXAMPLE_CONF_DIR?

@quazgar
Copy link

quazgar commented Feb 18, 2017

Tested myself and works for me.

@Anth0rx
Copy link
Author

Anth0rx commented Feb 27, 2017

Is this okay now and can be merged?

@quazgar
Copy link

quazgar commented Mar 4, 2017

I am not sure about @SoapGentoo's comment about setting HTML_DOCS, if that's critical. And to me, the easiest solution about the Irrlicht dependency for a dedicated server seems to be to just include it into the DEPENDS regardless of the dedicated USE flag. SoapGentoo, do you think that these changes would be sufficient?

@Anth0rx
Copy link
Author

Anth0rx commented Apr 12, 2017

What is this qa-bot thingy with those app-leechcraft problems?

@soredake
Copy link
Contributor

soredake commented Jun 5, 2017

Any progress?

@Anth0rx
Copy link
Author

Anth0rx commented Jun 24, 2017

I doubt the breakages were added by the pull request. How would you recommend doing the rebase?

@soredake
Copy link
Contributor

@Anth0rx git pull upstream master -r

@Sur3
Copy link

Sur3 commented Jun 28, 2017

Should also be updated to 0.4.16 I suppose.

@Anth0rx
Copy link
Author

Anth0rx commented Jun 28, 2017

Yes it should. So it will be a new "bug" at https://bugs.gentoo.org followed by a new Pull Request here on GitHub?

@Sur3
Copy link

Sur3 commented Jun 28, 2017

I suppose just a new pull request is ok, no second bug needed. ;)

@mgorny
Copy link
Member

mgorny commented Jul 5, 2017

No need for a new PR. Just update this one.

@Anth0rx
Copy link
Author

Anth0rx commented Aug 4, 2017

So I executed the git pull upstream master -r command. Unfortunately now it looks a bit messy with all those commits here in the PR. Was is supposed to be like this?

@Anth0rx Anth0rx changed the title games-action/minetest: version bump to 0.4.15 games-action/minetest: version bump to 0.4.16 Aug 4, 2017
@soredake
Copy link
Contributor

soredake commented Aug 4, 2017

@Anth0rx do git reset --hard 096f97d95f7d85ce76f04c6446aa04f6b5620bc9 then git pull -r upstream master again.

@Anth0rx
Copy link
Author

Anth0rx commented Aug 8, 2017

So, should I now squash the commits together?

@soredake
Copy link
Contributor

soredake commented Aug 9, 2017

Yes, squash and leave only 0.4.16 version and remove '# $Id$' header from ebuild.

@Anth0rx
Copy link
Author

Anth0rx commented Aug 9, 2017

Why can't I leave the 0.4.15 version? It is functioning after all. I could also add the live-ebuild of the development version of Minetest.

Do you know how to properly test ebuilds? Since I solely tested them on my system with my USE flags.
I thought of some Stage-3 docker image which is completely stock. Or maybe a chroot? But I have never found out some generic way to do so.

@mgorny
Copy link
Member

mgorny commented Aug 10, 2017

We do not add interim versions unless there is a very good reason to. If you are adding both at the same time, people will just use the newer and the older one will get no testing. Eventually, it will be removed as cleanup, so you're only making double work.

DEPEND="${RDEPEND}
>=dev-games/irrlicht-1.8-r2
doc? (
app-doc/doxygen
Copy link
Member

Choose a reason for hiding this comment

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

Misindent. But didn't I report that already somewhere?

cmake-utils_src_prepare
# set paths
sed \
-e "s#@BINDIR@#/usr/bin#g" \
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this take EPREFIX too?

Copy link
Author

Choose a reason for hiding this comment

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

Where should I put the EPREFIX?

Copy link
Member

Choose a reason for hiding this comment

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

Before /usr, just like the path in mycmakeargs.


src_configure() {
local mycmakeargs=(
-DBUILD_CLIENT=$(usex !dedicated)
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 prefer explicit USE=client than this negative logic but I can live with it if you insist.

src_configure() {
local mycmakeargs=(
-DBUILD_CLIENT=$(usex !dedicated)
-DCUSTOM_BINDIR="${EPREFIX}/usr/bin"
Copy link
Member

Choose a reason for hiding this comment

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

Are all those overrides really necessary? i.e. doesn't the game respect CMAKE_INSTALL_PREFIX and so on?

Copy link
Author

Choose a reason for hiding this comment

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

That's a good question. Since I simply did a version bump, leaving merely everything as it was, I don't know what the previous maintainer had in mind.

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to leave it as-is, and make yourself a note to verify it in the future.

Copy link
Author

Choose a reason for hiding this comment

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

Okay.

)

use dedicated && mycmakeargs+=(
-DIRRLICHT_INCLUDE_DIR=${EPREFIX}/usr/include/irrlicht
Copy link
Member

Choose a reason for hiding this comment

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

Tab-indent, please.

}

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.

@Anth0rx Anth0rx force-pushed the minetest-bump branch 2 times, most recently from 8df91fc to e15337e Compare August 17, 2017 21:43
@Anth0rx
Copy link
Author

Anth0rx commented Aug 20, 2017

I merged the changes from the minetest_game (PR #3875) branch into this one. Thus the QA problems caused by the games-action/minetest_game dependency to games-action/minetest are avoided.

@mgorny
Copy link
Member

mgorny commented Aug 21, 2017

There is still the comment wrt EPREFIX in sed that needs addressing (you can see it in the 'files changed' tab). Also, please either prefix the fix commits with package name, or squash them into the original commit.

@soredake
Copy link
Contributor

Ping.

@Anth0rx
Copy link
Author

Anth0rx commented Aug 29, 2017

Ping.
?

The 'client' use flag was not defined so I replaced it by the original
'!dedicated' flag.
@Anth0rx
Copy link
Author

Anth0rx commented Aug 29, 2017

So I just removed the invalid USE flag 'client'. Now it builds again. If it is okay on your side this PR can be merged I think (after rebasing).

@gentoo-repo-qa-bot
Copy link
Collaborator

😞 The QA check for this pull request has found the following issues:

Issues inherited from Gentoo (may be modified by PR):
https://qa-reports.gentoo.org/output/gentoo-ci/84c1740c8/output.html#media-sound/snd
https://qa-reports.gentoo.org/output/gentoo-ci/84c1740c8/output.html#sys-devel/binutils-hppa64

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.

Why don't you use repoman?

RepoMan scours the neighborhood...
  ebuild.minorsyn               1
   games-action/minetest/minetest-0.4.16.ebuild: Unquoted Variable on line: 87

Note: use --include-dev (-d) to check dependencies for 'dev' profiles

RepoMan sez: "You're only giving me a partial QA payment?
              I'll take it this time, but I'm not happy."

I'm building it now.

@@ -64,7 +64,7 @@ src_prepare() {

Copy link
Member

Choose a reason for hiding this comment

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

Squash this commit.

@mgorny mgorny self-assigned this Sep 2, 2017
@mgorny mgorny added the fix on merge There is at least one issue still needing fixing. Please read the comments and fix it while merging. label Sep 2, 2017
@mgorny
Copy link
Member

mgorny commented Sep 2, 2017

I'm going to fix it and merge.

@gentoo-bot gentoo-bot closed this in 11c1ff4 Sep 2, 2017
strahlc pushed a commit to strahlc/gentoo that referenced this pull request Sep 6, 2017
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). fix on merge There is at least one issue still needing fixing. Please read the comments and fix it while merging. maintainer-needed There is at least one affected package with no maintainer. Review it if you can.
Projects
None yet
8 participants