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-board/cockatrice: version bump to 20181220 #11149

Conversation

GuillaumeSeren
Copy link
Contributor

Closes: https://bugs.gentoo.org/664712
Closes: https://bugs.gentoo.org/678690
Package-Manager: Portage-2.3.51, Repoman-2.3.11

@gentoo-bot
Copy link

Copyright policy change

Please note that on 2018-09-15 Trustees have approved new Gentoo copyright policy. All contributions made to Gentoo need to follow this policy. If you include the Signed-off-by line in your commit message, you indicate that you have read the policy and agree to its terms. For more detailed explanation, please see the new Gentoo copyright policy explained article.

Pull Request assignment

Submitter: @GuillaumeSeren
Areas affected: ebuilds
Packages affected: games-board/cockatrice

games-board/cockatrice: @gentoo/games

Linked bugs

Bugs linked: 664712, 678690

Missing GCO sign-off

Please read the terms of Gentoo Certificate of Origin and acknowledge them by adding a sign-off to all your commits.


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). bug linked Bug/Closes found in footer, and cross-linked with the PR. no signoff One or more commits do not indicate GCO sign-off. labels Feb 24, 2019
@GuillaumeSeren GuillaumeSeren force-pushed the games-board/cockatrice-add-version-bump-20181220 branch from bd4c33d to 164a9ea Compare February 26, 2019 13:13
@GuillaumeSeren
Copy link
Contributor Author

Hey,
I have addedd the Signed-off-by tag in the commit, please tell me if there is a problem.

@GuillaumeSeren GuillaumeSeren force-pushed the games-board/cockatrice-add-version-bump-20181220 branch from 164a9ea to d80a9b4 Compare March 2, 2019 09:40
@GuillaumeSeren
Copy link
Contributor Author

Hey,
I have rebased the branch, I will be around if any changes are needed.

@GuillaumeSeren GuillaumeSeren force-pushed the games-board/cockatrice-add-version-bump-20181220 branch from d80a9b4 to 10236b7 Compare March 9, 2019 09:10
@GuillaumeSeren
Copy link
Contributor Author

Hey,
I rebased the branch.

@GuillaumeSeren GuillaumeSeren force-pushed the games-board/cockatrice-add-version-bump-20181220 branch from 10236b7 to 493bfa4 Compare March 9, 2019 09:39
@GuillaumeSeren
Copy link
Contributor Author

Hello @chewi I have heard on irc,
that you might be interrested in this, can you help me ?

@chewi
Copy link
Member

chewi commented Mar 9, 2019

Sorry for the wait. I don't have time to deal with every game. 😞 Unfortunately 2.7.0 came out a few days ago! Some other points to address:

  • dev-qt/linguist-tools should be in BDEPEND, not DEPEND.
  • The directories in src_configure should include ${EPREFIX}.
  • The build is unconditionally using ccache, which is bad. There doesn't seem to be a way to turn this off without patching. A patch to remove this would be appreciated. An option to disable it that could be upstreamed would be even better.

@GuillaumeSeren
Copy link
Contributor Author

Hey @chewi thank you for the quick answer.

I don't have time to deal with every game

Yes sorry to HL you in particular but someone give your name on irc (hey @juippis), so I try'ed.

Unfortunately 2.7.0 came out a few days ago!

Yes let's aggre on the main issue on this version, then I'll bump it.

dev-qt/linguist-tools should be in BDEPEND, not DEPEND.

I Never used this before, but I find some doc on EAPI7 guide.

The directories in src_configure should include ${EPREFIX}.

Never used that also, I upgraded like:

		-DICONDIR="${EPREFIX}/usr/share/icons"
		-DDESKTOPDIR="${EPREFIX}/usr/share/applications" )

The build is unconditionally using ccache, which is bad. There doesn't seem to be a way to turn this off without patching. A patch to remove this would be appreciated. An option to disable it that could be upstreamed would be even better.

This does not seems so trivial, I will check the sources tomorrow but if you have any pointer (or similar patch) that could help me working on that.

@GuillaumeSeren GuillaumeSeren force-pushed the games-board/cockatrice-add-version-bump-20181220 branch from 493bfa4 to 9e42a74 Compare March 10, 2019 18:36
@chewi
Copy link
Member

chewi commented Mar 10, 2019

Thanks for that.

Sorry, I've just realised protobuf needs to be in all three depends, including BDEPEND. protoc is executed during the build. I hope it's platform-agnostic! I wondered whether the SLOT operator should be :0= but that's debatable at this point so leave it be.

Regarding CMake, I'm no expert but you could add an OPTION called USE_CCACHE that defaults to ON and wrap the existing ccache stuff in a conditional. If it's too tricky, let me know and I'll handle it.

@GuillaumeSeren
Copy link
Contributor Author

Well thank you for the feedback,

Sorry, I've just realised protobuf needs to be in all three depends, including BDEPEND. protoc is executed during the build. I hope it's platform-agnostic! I wondered whether the SLOT operator should be :0= but that's debatable at this point so leave it be.

Ok done with.

BDEPEND="
	dev-qt/linguist-tools:5
	client? || server?(
		dev-libs/protobuf:=
	)"

Regarding CMake, I'm no expert but you could add an OPTION called USE_CCACHE that defaults to ON and wrap the existing ccache stuff in a conditional. If it's too tricky, let me know and I'll handle it.

Well I checked the cockatrice source about the ccache stuff,
what do you think ?

	# Disable the ccache if not activated
	if ! has ccache ${FEATURES}; then
		sed -e 's/^export CCACHE_CPP2=true/export CCACHE_CPP2=false/' -i cmake/launch-c.in
		sed -e 's/^export CCACHE_CPP2=true/export CCACHE_CPP2=false/' -i cmake/launch-cxx.in
	fi

Guillaume

@GuillaumeSeren GuillaumeSeren force-pushed the games-board/cockatrice-add-version-bump-20181220 branch from 9e42a74 to e2c9a35 Compare March 11, 2019 13:50
Closes: https://bugs.gentoo.org/664712
Closes: https://bugs.gentoo.org/678690
Package-Manager: Portage-2.3.51, Repoman-2.3.11
Signed-off-by: Guillaume Seren <guillaumeseren@gmail.com>
@GuillaumeSeren GuillaumeSeren force-pushed the games-board/cockatrice-add-version-bump-20181220 branch from e2c9a35 to ddb342a Compare March 12, 2019 09:44
@GuillaumeSeren
Copy link
Contributor Author

Hello @chewi ,
I have upgraded the branch if you want to check it up.

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2019-03-12 10:19 UTC
Newest commit scanned: ddb342a
Status: ✅ good

No issues found

@chewi
Copy link
Member

chewi commented Mar 13, 2019

Merged now. You evidently didn't test that last change though, your use of || was invalid syntax. 😉 I took the liberty of fixing that and removing := from protobuf in BDEPEND as it doesn't make sense there. The ccache fix wasn't right I'm afraid but I dealt with that too. Hopefully upstream will take my patch. I also bumped to the latest version. Thanks for your help!

@GuillaumeSeren
Copy link
Contributor Author

@chewi Ah yes I made the changes in a hurry and didn't the time to test it,
thank you for your help I will do better next time.

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). bug linked Bug/Closes found in footer, and cross-linked with the PR. no signoff One or more commits do not indicate GCO sign-off.
Projects
None yet
4 participants