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

lxd: Setup image and target arch for cross-compilation #1286

Merged
merged 3 commits into from May 8, 2017

Conversation

@kalikiana
Copy link
Contributor

@kalikiana kalikiana commented Apr 27, 2017

To support cross-compilation in LXD containers the image for the host architecture needs to be used, rather than for the target architecture, which would require slow qemu emulation. It's worth noting that host here means the host running LXD which may not be the same as the host running snapcraft eg. snapcraft executed on an armhf machine using an amd64 remote building for arm64.

Note: this is orthogonal to actual support for cross-compilation in individual plugins and only deals with the container setup.

@codecov-io
Copy link

@codecov-io codecov-io commented Apr 27, 2017

Codecov Report

Merging #1286 into master will decrease coverage by 0.05%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1286      +/-   ##
==========================================
- Coverage   95.27%   95.21%   -0.06%     
==========================================
  Files         218      220       +2     
  Lines       20282    20454     +172     
  Branches     1616     1631      +15     
==========================================
+ Hits        19324    19476     +152     
- Misses        677      689      +12     
- Partials      281      289       +8
Impacted Files Coverage Δ
snapcraft/tests/test_lxd.py 100% <100%> (ø) ⬆️
snapcraft/_options.py 82.65% <100%> (+0.36%) ⬆️
snapcraft/tests/fixture_setup.py 70.92% <100%> (+0.18%) ⬆️
snapcraft/internal/lxd.py 88.73% <84.61%> (-0.42%) ⬇️
snapcraft/plugins/kbuild.py 90% <0%> (-4.74%) ⬇️
snapcraft/internal/sources/__init__.py 82.05% <0%> (-1.95%) ⬇️
snapcraft/internal/parts.py 99.06% <0%> (-0.47%) ⬇️
snapcraft/tests/plugins/test_kbuild.py 100% <0%> (ø) ⬆️
snapcraft/tests/test_project_loader.py 100% <0%> (ø) ⬆️
snapcraft/tests/sources/test_7z.py 100% <0%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 463f1e8...71ac0fc. Read the comment docs.

@@ -38,6 +40,11 @@

class Containerbuild:

def _get_server_info(self):

This comment has been minimized.

@elopio

elopio May 2, 2017
Contributor

Another nit, can you move this method after init?
I love some of the suggestions from the clean code book. One of those is vertical organization, you put first the caller method, and after the called method. This leaves the functions close to were they were called first, but the nicest thing for me is that you can read a source code file from top to bottom. In here if you find _get_server_info you have no context about what it does. If you instead find init first, you will see the context where _get_server_info is called and then you can decide if the internals of that method are relevant to what you are looking for, or you can just skip it.

@@ -38,6 +40,11 @@

class Containerbuild:

def _get_server_info(self):

This comment has been minimized.

@elopio

elopio May 2, 2017
Contributor

shouldn't it be called _get_remote_info instead?

@@ -46,20 +47,27 @@ def test_cleanbuild(self, mock_pet):

mock_pet.return_value = 'my-pet'

project_options = ProjectOptions()
with patch('platform.machine') as machine_mock, \

This comment has been minimized.

@elopio

elopio May 2, 2017
Contributor

nit: prefer parentheses instead of splitting lines with \

This comment has been minimized.

@kalikiana

kalikiana May 3, 2017
Author Contributor

I don't see how that's possible here. But I figured I can use decorators anyway so there's no need for the with at all.

Copy link
Contributor

@elopio elopio left a comment

lgtm. I just left a couple of style comments.

@elopio
elopio approved these changes May 3, 2017
Copy link
Contributor

@elopio elopio left a comment

Thanks for making the changes for my pita comments :D

@sergiusens sergiusens merged commit 6a73722 into snapcore:master May 8, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@@ -52,6 +54,19 @@ def __init__(self, *, output, source, project_options,
remote = _get_default_remote()
_verify_remote(remote)
self._container_name = '{}:snapcraft-{}'.format(remote, container_name)
# Use the server architecture to avoid emulation overhead
kernel = self._get_remote_info()['environment']['kernel_architecture']

This comment has been minimized.

@elopio

elopio May 10, 2017
Contributor

@kalikiana this causes a regresion on xenial: https://bugs.launchpad.net/snapcraft/+bug/1689712
I've assigned the bug to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.