/snapcraft

Add qmake plugin. #566

Merged
merged 14 commits into from Jun 24, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@kyrofa @sergiusens @elopio @lpotter @8none1
@kyrofa
Member

kyrofa commented Jun 10, 2016

This PR resolves LP: #1574774 with a new qmake plugin. It also adds a qt4 and qt5 demo to make use of it.
@kyrofa
Member

kyrofa commented Jun 10, 2016

Hmm... main.py has apparently gotten complex.
@kyrofa
Add qmake plugin. 
Also add a qt4 and qt5 demo to make use of it.

LP: #1574774

Signed-off-by: Kyle Fazzari <kyle@canonical.com>
This commit was signed with a verified signature.
@kyrofa kyrofa Kyle Fazzari
GPG key ID: A7D23FB0E778DEDF Learn about signing commits

Some checks were not successful

2 failing and 2 successful checks
@snappy-m-o
Examples tests
Details
@snappy-m-o
autopkgtest
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.07%) to 95.814%
Details
a1830dc
@sergiusens
Collaborator

sergiusens commented Jun 10, 2016

El 10/06/16 a las 17:15, Kyle Fazzari escribió:

Hmm... main.py has apparently gotten complex.
With this branch?
snapcraft/plugins/qmake.py
@@ -0,0 +1,106 @@
+# -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*-
+#
+# Copyright (C) 2015 Canonical Ltd
@elopio

elopio Jun 10, 2016

Member

2016

@kyrofa

kyrofa Jun 11, 2016

Member

Fixed.

snapcraft/plugins/qmake.py
+
+ - options:
+ (list of strings)
+ additional options to pass to the qmake invocation.
@elopio

elopio Jun 10, 2016

Member

qt-version too, right?

@kyrofa

kyrofa Jun 11, 2016

Member

Ah yes, thank you. Fixed.

snapcraft/plugins/qmake.py
+ schema['build-properties'].append('options')
+
+ # Inform Snapcraft of the properties associated with pulling. If these
+ # change in the YAML Snapcraft will consider the pull step dirty.
@elopio

elopio Jun 10, 2016

Member

I think you shouldn't duplicate the comment, just move the second statement right under the first.

@kyrofa

kyrofa Jun 11, 2016

Member

Yeah, a little verbose. Fixed.

snapcraft/plugins/qmake.py
+
+ def _build_environment(self):
+ env = os.environ.copy()
+ env['QT_SELECT'] = self.options.qt_version
@elopio

elopio Jun 10, 2016

Member

cool, I think I'm going to like this.

@sergiusens

sergiusens Jun 15, 2016

Collaborator

yeah, this will play nicely with the upcoming build-environment keyword.

snapcraft/tests/test_plugin_qmake.py
+
+# -*- Mode:Python; indent-tabs-mode:nil; tab-width:4 -*-
+#
+# Copyright (C) 2015 Canonical Ltd
@elopio

elopio Jun 10, 2016

Member

2016

@kyrofa

kyrofa Jun 11, 2016

Member

Fixed.

snapcraft/tests/test_plugin_qmake.py
+ '"string", but it was "{}"'
+ .format(options_items_type))
+
+ # Check install-via property
@elopio

elopio Jun 10, 2016

Member

s/install-via/qt-version

@kyrofa

kyrofa Jun 11, 2016

Member

Good catch, thank you. Fixed.

snapcraft/tests/test_plugin_qmake.py
+ os.makedirs(plugin.builddir)
+ plugin.build()
+
+ expected = {'QT_SELECT': 'qt5'}
@elopio

elopio Jun 10, 2016

Member

no test for qt4? You can make one test, with two subTests:
https://docs.python.org/3/library/unittest.html#distinguishing-test-iterations-using-subtests

@kyrofa

kyrofa Jun 11, 2016

Member

Ah ha! I was totally intending to do this and apparently forgot, thank you. Fixed.

@elopio
Member

elopio commented Jun 10, 2016

I don't see a test for the unsupported qt version error.
And the demos could be tested for now just checking that you can get and install the snap. We can leave the execution for later, maybe report a bug saying that we need tests with xvfb.
@kyrofa
Member

kyrofa commented Jun 11, 2016

Hmm... main.py has apparently gotten complex.

With this branch?

No, I'm getting that on master as well.
@kyrofa
Add qt4/qt5 demos to demos_tests. 
Also add a few more qmake plugin tests to up coverage to 100%.

Signed-off-by: Kyle Fazzari <kyle@canonical.com>
This commit was signed with a verified signature.
@kyrofa kyrofa Kyle Fazzari
GPG key ID: A7D23FB0E778DEDF Learn about signing commits

Some checks were not successful

2 failing and 2 successful checks
@snappy-m-o
Examples tests
Details
@snappy-m-o
autopkgtest
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.1%) to 95.849%
Details
c462ec6
@kyrofa
Member

kyrofa commented Jun 11, 2016

I don't see a test for the unsupported qt version error.

Good catch, I added one.

And the demos could be tested for now just checking that you can get and install the snap. We can leave the execution for later, maybe report a bug saying that we need tests with xvfb.

Ah, thank you I forgot about actually adding them to demos tests-- done. We'll see if they work! Regarding the xvfb tests, I created a bug for you 😉 .

kyrofa added some commits Jun 13, 2016

@kyrofa
Fix demo tests. 
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
This commit was signed with a verified signature.
@kyrofa kyrofa Kyle Fazzari
GPG key ID: A7D23FB0E778DEDF Learn about signing commits

Some checks were not successful

1 failing and 2 successful checks
@snappy-m-o
autopkgtest
Details
@snappy-m-o
Examples tests — Success
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
4b02af3
@kyrofa
Merge branch 'master' into feature/1574774/qmake_plugin

Some checks were not successful

1 failing and 3 successful checks
@snappy-m-o
autopkgtest
Details
@snappy-m-o
Examples tests — Success
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.002%) to 95.8%
Details
5ec5c47
@elopio
Member

elopio commented Jun 13, 2016

testtools.matchers._impl.MismatchError: !=:
reference = '''
ant catkin copy jdk kernel maven nodejs python3 tar-content
autotools cmake go kbuild make nil python2 scons
'''
actual = '''
ant catkin copy jdk kernel maven nodejs python3 scons
autotools cmake go kbuild make nil python2 qmake tar-content
'''
@kyrofa
Fix list-plugins integration test. 
Signed-off-by: Kyle Fazzari <kyle@canonical.com>

Some checks were not successful

2 failing and 2 successful checks
@snappy-m-o
Examples tests
Details
@snappy-m-o
autopkgtest
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.05%) to 95.849%
Details
e2bcb37
@kyrofa
Member

kyrofa commented Jun 13, 2016

Should be fixed now. We have a lot of tests that test list-plugins!
@elopio
Member

elopio commented Jun 13, 2016

File a bug so we leave only one.
demos/qt4-text-editor/src/main.cpp
@@ -0,0 +1,59 @@
+/****************************************************************************
+**
+** Copyright (C) 2015 The Qt Company Ltd.
@sergiusens

sergiusens Jun 14, 2016

Collaborator

This will need an update in debian/copyright

@kyrofa

kyrofa Jun 15, 2016

Member

Ooo, good catch.

@kyrofa

kyrofa Jun 15, 2016

Member

Alright, take a look.

@sergiusens
Collaborator

sergiusens commented Jun 15, 2016

Please update the branch
@kyrofa
Merge remote-tracking branch 'origin/master' into feature/1574774/qma… 
…ke_plugin

Signed-off-by: Kyle Fazzari <kyle@canonical.com>
This commit was signed with a verified signature.
@kyrofa kyrofa Kyle Fazzari
GPG key ID: A7D23FB0E778DEDF Learn about signing commits

Some checks were not successful

1 failing and 3 successful checks
@snappy-m-o
autopkgtest
Details
@snappy-m-o
Examples tests — Success
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.05%) to 95.917%
Details
7fea1bc
@kyrofa
Member

kyrofa commented Jun 15, 2016

Please update the branch

Done. If I have to update the list-plugins tests again I think I'll collapse.
@kyrofa
Update debian copyright to cover Qt examples. 
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
This commit was signed with a verified signature.
@kyrofa kyrofa Kyle Fazzari
GPG key ID: A7D23FB0E778DEDF Learn about signing commits

Some checks were not successful

2 failing and 2 successful checks
@snappy-m-o
Examples tests
Details
@snappy-m-o
autopkgtest
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.05%) to 95.917%
Details
99b69f0
@elopio
Member

elopio commented Jun 15, 2016

retest this please
@kyrofa
Merge branch 'master' into feature/1574774/qmake_plugin

Some checks were not successful

2 failing and 2 successful checks
@snappy-m-o
Examples tests
Details
@snappy-m-o
autopkgtest
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.05%) to 95.917%
Details
8bea843
@kyrofa
Member

kyrofa commented Jun 15, 2016

I just added support for a list of project files (Will Cooke needed it). Waiting to hear back regarding his testing.

kyrofa added some commits Jun 15, 2016

@kyrofa
Qmake plugin: support list of project files. 
Signed-off-by: Kyle Fazzari <kyle@canonical.com>

Some checks were not successful

2 failing and 2 successful checks
@snappy-m-o
autopkgtest
Details
continuous-integration/travis-ci/pr — The Travis CI build failed
Details
@snappy-m-o
Examples tests — Success
Details
coverage/coveralls — Coverage increased (+0.07%) to 95.932%
Details
edb79b3
@kyrofa
Add support for qmake's CPATH and INCLUDE_PATH. 
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
This commit was signed with a verified signature.
@kyrofa kyrofa Kyle Fazzari
GPG key ID: A7D23FB0E778DEDF Learn about signing commits

Some checks were not successful

2 failing and 2 successful checks
@snappy-m-o
Examples tests
Details
@snappy-m-o
autopkgtest
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.07%) to 95.936%
Details
e90bb94
snapcraft/internal/yaml.py
+ library_path = _get_library_paths(
+ 'LIBRARY_PATH', root, arch_triplet, prepend='', sep=':')
+ if library_path:
+ env.append(library_path)
@kyrofa

kyrofa Jun 16, 2016

Member

@sergiusens @elopio may I draw your attention here. qmake only seems to respect these flags instead of the gcc ones. I had to put them here because I didn't want to duplicate the _get_*_paths() logic in the plugin, and that logic is not public. Is this okay? Or should we make those functions part of the public API?

@lpotter

lpotter commented Jun 16, 2016

Is there support for specifying arguments for CONFIG ? I am needing to be able to do this with a couple of snaps I am working on.
@8none1

8none1 commented Jun 17, 2016

Confirming that this qmake plugin has successfully built my project. Thanks!
@kyrofa
Member

kyrofa commented Jun 17, 2016

Thanks @8none1!

@lpotter yes you should be able to do that via the options parameter, but please let me know if you run into problems.

kyrofa added some commits Jun 24, 2016

@kyrofa
Merge branch 'master' into feature/1574774/qmake_plugin 
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
This commit was signed with a verified signature.
@kyrofa kyrofa Kyle Fazzari
GPG key ID: A7D23FB0E778DEDF Learn about signing commits

Some checks were not successful

2 failing, 1 pending, and 1 successful checks
@snappy-m-o
autopkgtest
Details
continuous-integration/travis-ci/pr — The Travis CI build failed
Details
@snappy-m-o
Examples tests — Started
Details
coverage/coveralls — Coverage increased (+0.06%) to 96.176%
Details
200dc72
@kyrofa
Make get_{include,library}_paths part of the public API. 
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
This commit was signed with a verified signature.
@kyrofa kyrofa Kyle Fazzari
GPG key ID: A7D23FB0E778DEDF Learn about signing commits

Some checks were not successful

3 failing and 1 successful checks
@snappy-m-o
Examples tests
Details
@snappy-m-o
autopkgtest
Details
continuous-integration/travis-ci/pr — The Travis CI build failed
Details
coverage/coveralls — Coverage increased (+0.06%) to 96.177%
Details
b3bf274
snapcraft/plugins/qmake.py
+ def _extra_config(self):
+ extra_config = []
+
+ for root in [self.installdir, self.project.stage_dir]:
@kyrofa

kyrofa Jun 24, 2016

Member

@sergiusens @elopio here's the side effect of moving this into the plugin: the plugin doesn't know whether or not it's a root part or a dependent part, so it needs to look in both installdir and stagedir for libs/includes. Do either of you see a downside to doing this?

@sergiusens

sergiusens Jun 24, 2016

Collaborator

should be mostly fine as this is what we do in the machinery already. Just need to make sure installdir is preferred over stagedir.

@kyrofa

kyrofa Jun 24, 2016

Member

Alright very good, I thought the same.

@kyrofa
Member

kyrofa commented Jun 24, 2016

Alright, this is ready for another look.
@kyrofa
Move libs/includes to qmake plugin. 
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
This commit was signed with a verified signature.
@kyrofa kyrofa Kyle Fazzari
GPG key ID: A7D23FB0E778DEDF Learn about signing commits

Some checks haven’t completed yet

2 pending and 2 successful checks
@snappy-m-o
Examples tests — Triggered
@snappy-m-o
autopkgtest — Triggered
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.06%) to 96.175%
Details
6016e8f
snapcraft/internal/common.py
+
+def format_path_variable(envvar, paths, prepend, separator):
+ if not paths:
+ return ''
@sergiusens

sergiusens Jun 24, 2016

Collaborator

So in the uses of format_path_variable I see you always check for path, so instead wouldn't it be better here to raise a ValueError?

@kyrofa

kyrofa Jun 24, 2016

Member

Ah, yeah good catch. Honestly though I don't think we need to check it at all here-- if the caller asked to format an environment variable out of the paths, they should probably know that the paths are good, otherwise there's nothing reasonable to return. If you want ValueError raised though, that's fine too.

@kyrofa

kyrofa Jun 24, 2016

Member

Meh... that's just asking for cryptic errors. I'll raise.

@kyrofa

kyrofa Jun 24, 2016

Member

Done.

@sergiusens

sergiusens Jun 24, 2016

Collaborator

El 24/06/16 a las 14:49, Kyle Fazzari escribió:

In snapcraft/internal/common.py
ubuntu-core#566 (comment):

  •    os.path.join(root, 'usr', 'lib'),

  •    os.path.join(root, 'lib', arch_triplet),

  •    os.path.join(root, 'usr', 'lib', arch_triplet),
  • ]
  • return [p for p in paths if os.path.exists(p)]

+def combine_paths(paths, prepend, separator):

  • paths = ['{}{}'.format(prepend, p) for p in paths]
  • return separator.join(paths)

+def format_path_variable(envvar, paths, prepend, separator):

  • if not paths:
  •    return ''

Ah, yeah good catch. Honestly though I don't think we need to check it
at all here-- if the caller asked to format an environment variable out
of the paths, they should probably know that the paths are good,
otherwise there's nothing reasonable to return. If you want ValueError
raised though, that's fine too.

There's nothing wrong with raising a nice exception so the user of the
API can fix fast ;-)

@sergiusens
Collaborator

sergiusens commented Jun 24, 2016

ok @kyrofa, good stuff, just a comment on a check going on.
@kyrofa
Raise if format_path_variable is handed empty paths. 
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
This commit was signed with a verified signature.
@kyrofa kyrofa Kyle Fazzari
GPG key ID: A7D23FB0E778DEDF Learn about signing commits

All checks have passed

2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.06%) to 96.175%
Details
40c1477
@kyrofa
Member

kyrofa commented Jun 24, 2016

good stuff, just a comment on a check going on.

Very good-- fixed.

@kyrofa kyrofa merged commit 07581cb into snapcore:master Jun 24, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.06%) to 96.175%
Details

@kyrofa kyrofa deleted the kyrofa:feature/1574774/qmake_plugin branch Jun 24, 2016

kalikiana pushed a commit to kalikiana/snapcraft that referenced this pull request Apr 6, 2017

@kyrofa
Add qmake plugin. (#566) 
Also add a qt4 and qt5 demo to make use of it.

LP: #1574774

Signed-off-by: Kyle Fazzari <kyle@canonical.com>
98efc94
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment