/snapcraft

New plugin: kernel #366

Merged
merged 1 commit into from Mar 21, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@sergiusens @coveralls @kyrofa
@sergiusens
Collaborator

sergiusens commented Mar 3, 2016

Includes 2 plugins with an example for each.

Jointly developed by Alexander Sack and Sergio Schvezov

LP: #1552168

Signed-off-by: Sergio Schvezov sergio.schvezov@ubuntu.com
snapcraft/common.py
'armv7l': {
- 'arch': 'armhf',
+ 'kernel': 'arm',
+ 'deb': 'armhf',
@kyrofa

kyrofa Mar 10, 2016

Member

Eww, why can't people just agree? 😛

@sergiusens

sergiusens Mar 16, 2016

Collaborator

What, how is that possible? 😄

If everyone agreed we'd be jobless 😉

snapcraft/common.py
@@ -98,20 +103,36 @@ def __init__(self):
super().__init__(
'{0} is not supported, please log a bug at '
'https://bugs.launchpad.net/snapcraft/+filebug?'
- 'field.title=please+add+support+for+{0}'.format(
- platform.machine()))
+ 'field.title=please+add+support+for+{0}'.format(_machine))
@kyrofa

kyrofa Mar 10, 2016

Member

I know you didn't add the title in this PR, but I wanted to voice a small concern. We're introducing a dependency here on Launchpad's URL format. Do we know that will remain stable? What happens in a few years when this code is still used but Launchpad renames title to be summary for example? We'll still be printing this but people will get a 404.

@sergiusens

sergiusens Mar 16, 2016

Collaborator

This is stable. If it comes to it, it would need to be changed like any other link we have.

snapcraft/plugins/kbuild.py
+provided by that plus the following kbuild specific options with semantics as
+explained above:
+
+ - kdefconfig:
@kyrofa

kyrofa Mar 10, 2016

Member

I suggest reversing the order of listing the variables and the above description. Much of the above description mention the below variables by name but the reader has no context as to what those variables are until they get to the bottom.

@sergiusens

sergiusens Mar 16, 2016

Collaborator

ack

snapcraft/plugins/kbuild.py
+ explicit list of configs to force; this will override the configs that
+ were set as base through kdefconfig and kconfigfile and dependent configs
+ will be fixed using the defaults encoded in the kbuild config
+ definitions. If you dont want default for one or more implicit configs
@kyrofa

kyrofa Mar 10, 2016

Member

s/dont/don't/

snapcraft/plugins/kbuild.py
+ # note that prepending and appending the overrides seems
+ # only way to convince all kbuild versions to pick up the
+ # configs during oldconfig in .config
+ config = '{}\n\n{}'.format(config, '\n'.join(self.options.kconfigs))
@kyrofa

kyrofa Mar 10, 2016

Member

I'm not sure I understand what's going on here. Let's say self.options.kconfigs = [A] and the config_path contents is B. Lines 142-144 would result in:

A

B
A

Right? Then line 149 would result in:

A

B
A

A

What am I missing?

@sergiusens

sergiusens Mar 10, 2016

Collaborator

Hah, I thought I wasn't doing A B A but it turns out I was doing the header part while doing the read and the footer while doing the write. I'll fix this to do it all in one location.

snapcraft/plugins/kbuild.py
+ def do_remake_config(self):
+ # update config to include kconfig amendments using oldconfig
+ cmd = 'yes "" | make {} oldconfig'.format(
+ ' '.join(self.options.build_parameters))
@kyrofa

kyrofa Mar 10, 2016

Member

Referring to the subprocess security guidelines, consider using shlex.quote() here to escape user-supplied input due to the use of shell=True below.

@sergiusens sergiusens changed the title from Kernel and kbuild plugin to New plugin: kernel Mar 17, 2016

@coveralls

coveralls commented Mar 19, 2016

Coverage Status

Coverage remained the same at 93.68% when pulling 0a1f8a4 on sergiusens:feature/1552168/kernel-plugin into 3cc0049 on ubuntu-core:master.
@coveralls

coveralls commented Mar 19, 2016

Coverage Status

Coverage remained the same at 95.425% when pulling 1c5f8ea on sergiusens:feature/1552168/kernel-plugin into 3cc0049 on ubuntu-core:master.
@coveralls

coveralls commented Mar 19, 2016

Coverage Status

Coverage remained the same at 95.425% when pulling 559044d on sergiusens:feature/1552168/kernel-plugin into 3cc0049 on ubuntu-core:master.
@sergiusens
Collaborator

sergiusens commented Mar 19, 2016

@kyrofa I would be pleased for a rereview of this :-)

unit test writing took me the entire day!
@coveralls

coveralls commented Mar 21, 2016

Coverage Status

Coverage remained the same at 95.425% when pulling f8fea60 on sergiusens:feature/1552168/kernel-plugin into 3cc0049 on ubuntu-core:master.
examples/96boards-kernel/snapcraft.yaml
+ - qcom/msm8916-mtp
+ 410c-firmware:
+ plugin: tar-content
+ source: firmware.tar # from developer.qualcomm.com v1.2
@kyrofa

kyrofa Mar 21, 2016

Member

I haven't quite woken up yet, but where is this coming from to make the example succeed?

@sergiusens

sergiusens Mar 21, 2016

Collaborator

from developer.qualcomm.com; you need to accept a license agreement.

@sergiusens

sergiusens Mar 21, 2016

Collaborator

FWIW anyone building such qualcomm kernel would know they need to do this so I didn't explain much. This will also be accompanied by a blog post (kernel plugin is subject to change).

@kyrofa

kyrofa Mar 21, 2016

Member

Ah, okay. I wish we could have a more complete example is all, I guess. One that a dev can just run instead of needing to hunt these down.

snapcraft/common.py
+ 'kernel': 'arm64',
+ 'deb': 'arm64',
+ 'cross-compiler-prefix': 'aarch64-linux-gnu-',
+ 'cross-build-packages': ['gcc-aarch64-linux-gnu'],
@kyrofa

kyrofa Mar 21, 2016

Member

Should we add these for other supported architectures as well?

@sergiusens

sergiusens Mar 21, 2016

Collaborator

we should, yes.

snapcraft/plugins/kernel.py
+
+
+_compression_command = {
+ 'gz': 'gzip',
@kyrofa

kyrofa Mar 21, 2016

Member

Don't the docs above state that bz2 and xz are also options here?

@sergiusens

sergiusens Mar 21, 2016

Collaborator

Ok, updated the docs

snapcraft/plugins/kernel.py
+ def schema(cls):
+ schema = super().schema()
+
+ schema['properties']['kernel-image-target'] = {
@kyrofa

kyrofa Mar 21, 2016

Member

Type, or target? Docs and code diverge here.

@sergiusens

sergiusens Mar 21, 2016

Collaborator

Updated the docs

snapcraft/plugins/kernel.py
+ 'default': [],
+ }
+
+ schema['properties']['kernel-device-trees'] = {
@kyrofa

kyrofa Mar 21, 2016

Member

Docs diverge here as well, plural versus singular.

@sergiusens

sergiusens Mar 21, 2016

Collaborator

Updated the docs

snapcraft/plugins/kernel.py
+ schema['properties']['kernel-initrd-compression'] = {
+ 'type': 'string',
+ 'default': 'gz',
+ 'enum': ['gz'],
@kyrofa

kyrofa Mar 21, 2016

Member

The docs say this can be one of none, gz, bz2, xz. Looks like it's only gz?

@sergiusens

sergiusens Mar 21, 2016

Collaborator

Updated the docs

@kyrofa
Member

kyrofa commented Mar 21, 2016

Other than some documentation issues this looks good to me, but I must admit it does go beyond my kernel experience. You may want ogra or ppisati to check this out.
@sergiusens
New plugin: kernel 
Includes a kernel plugin based out of the kbuild plugin
and also including cross compile support.

Jointly developed by Alexander Sack and Sergio Schvezov

LP: #1552168

Signed-off-by: Sergio Schvezov <sergio.schvezov@ubuntu.com>

Some checks were not successful

1 failing and 3 successful checks
@snappy-m-o
Examples tests — 14 tests run, 0 skipped, 2 failed.
Details
@snappy-m-o
autopkgtest — Success No test results found.
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage remained the same at 95.425%
Details
6da4ee6
@coveralls

coveralls commented Mar 21, 2016

Coverage Status

Coverage remained the same at 95.425% when pulling 6da4ee6 on sergiusens:feature/1552168/kernel-plugin into 3cc0049 on ubuntu-core:master.
@sergiusens
Collaborator

sergiusens commented Mar 21, 2016

I'm going to ignore those example errors:

|-- sergiusens@lindon:~/source/github.com/ubuntu-core/snapcraft (feature/1552168/kernel-plugin) --|
$ python3 -m examples_tests --filter busybox --ip 10.0.3.145 
Tests running...
Cleaning up for part "busybox"
Cleaning up staging area
Cleaning up snapping area
busybox_1.0_amd64.snap                                                                                      100%  468KB 468.0KB/s   00:00    

Ran 14 tests in 53.939s
OK

and

|-- sergiusens@lindon:~/source/github.com/ubuntu-core/snapcraft (feature/1552168/kernel-plugin) --|
$ python3 -m examples_tests --filter libpipeline --ip 10.0.3.145 
Tests running...
Cleaning up for part "libpipeline"
Cleaning up for part "pipelinetest"
pipelinetest_1.0_amd64.snap                                                                                 100%   52KB  52.0KB/s   00:00    

Ran 14 tests in 75.111s
OK

sergiusens added a commit that referenced this pull request Mar 21, 2016

@sergiusens
Merge pull request #366 from sergiusens/feature/1552168/kernel-plugin 
New plugin: kernel

All checks have passed

2 successful checks
continuous-integration/travis-ci/push — The Travis CI build passed
Details
coverage/coveralls — First build on master at 95.425%
Details
b7c1e8b

@sergiusens sergiusens merged commit b7c1e8b into snapcore:master Mar 21, 2016
3 of 4 checks passed

3 of 4 checks passed

Examples tests 14 tests run, 0 skipped, 2 failed.
Details
autopkgtest Success No test results found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 95.425%
Details

@sergiusens sergiusens deleted the sergiusens:feature/1552168/kernel-plugin branch Mar 21, 2016

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

@sergiusens
Merge pull request #366 from sergiusens/feature/1552168/kernel-plugin 
New plugin: kernel
d76db23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment