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

net-misc/seafile-client: Don't require QtTest #4489

Closed
wants to merge 1 commit into from
Closed

net-misc/seafile-client: Don't require QtTest #4489

wants to merge 1 commit into from

Conversation

moschlar
Copy link
Contributor

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull Request assignment

Areas affected: ebuilds
Packages affected: net-misc/seafile-client

net-misc/seafile-client: @moschlar, @gentoo/proxy-maint

@gentoo-repo-qa-bot gentoo-repo-qa-bot added self-maintained The PR changes only packages that are maintained by the submitter (i.e. no need to ask anybody else) assigned PR successfully assigned to the package maintainer(s). labels Apr 24, 2017
@@ -0,0 +1,49 @@
# Copyright 1999-2017 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.

Any reason not to remove -r0 while at it? I'd say this is safe enough patch to justify git mv.

dev-qt/linguist-tools:5"

src_prepare() {
epatch "${FILESDIR}/${P}-only-use-qttest-when-needed.patch"
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 eapply in EAPI 6.

export QT_SELECT=qt5
local mycmakeargs=(
-DBUILD_SHIBBOLETH_SUPPORT="$(usex shibboleth)"
)
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to support tests?

@moschlar
Copy link
Contributor Author

@mgorny: Thanks for your review!
Is conditional test support implemented correctly now?

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.

You tell me. Do the tests work for you? You are always supposed to test your packages with FEATURES=test.


src_prepare() {
use test || eapply "${FILESDIR}/${P}-only-use-qttest-when-needed.patch"
Copy link
Member

Choose a reason for hiding this comment

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

You want to apply this unconditionally. After all, there's a conditional inside.

@moschlar
Copy link
Contributor Author

They do:

Test phase

>>> Test phase: net-misc/seafile-client-6.0.4-r1
>>> Working in BUILD_DIR: "/var/tmp/portage/net-misc/seafile-client-6.0.4-r1/work/seafile-client-6.0.4_build"
ctest -j 2 --test-load 999
Test project /var/tmp/portage/net-misc/seafile-client-6.0.4-r1/work/seafile-client-6.0.4_build
    Start 1: test_server-info
    Start 2: test_utils
1/4 Test #2: test_utils .......................   Passed    0.10 sec
    Start 3: test_file-utils
2/4 Test #1: test_server-info .................   Passed    0.11 sec
3/4 Test #3: test_file-utils ..................   Passed    0.03 sec
    Start 4: test_stl
4/4 Test #4: test_stl .........................   Passed    0.03 sec
100% tests passed, 0 tests failed out of 4
Total Test time (real) =   0.17 sec
 * Tests succeeded.
>>> Completed testing net-misc/seafile-client-6.0.4-r1

I was merely asking about the conditional use flag since I couldn't find much in the devmanual or the Wiki about how to properly do it so I listened to my guts.

@mgorny
Copy link
Member

mgorny commented Apr 25, 2017

Ah, ok. That looks good then. However… could you look at build log? With the way the code is done I'm a little scared it might be linking the whole program to Qt5Test instead of just the tests…

@moschlar
Copy link
Contributor Author

@mgorny, you were absolutely right!
I've changed my patch (also submitted to upstream) to only include the Qt5Test library for the tests!

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.

Great work, thanks! I've left one more question for the Qt team; however, it's how the ebuilds worked before, so I'll merge it anyway. Worst case, we'll fix it later ;-).

@@ -43,6 +45,7 @@ src_configure() {
export QT_SELECT=qt5
Copy link
Member

Choose a reason for hiding this comment

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

@gentoo/qt, is this something we're supposed to do in ebuilds?

Copy link
Contributor

Choose a reason for hiding this comment

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

uhm definitely not. You must use eqmake5 or fix the build system to detect the correct Qt version. Most cmake-based packages that support both Qt4 and Qt5 expose an option to select between the two.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, you two!
I've added a patch that fixes the build system in #4497.
Since the project doesn't use qmake, I suppose this is the more correct way.
(Compilation only works with Qt5 - but if Qt4 is installed, there is one test for qmake version that then gives the result for Qt4...)

@mgorny mgorny self-assigned this Apr 26, 2017
@moschlar moschlar deleted the 615838 branch April 8, 2018 15:15
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). self-maintained The PR changes only packages that are maintained by the submitter (i.e. no need to ask anybody else)
Projects
None yet
4 participants