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

rust plugin: Add support for cross-compilation #1350

Merged
merged 15 commits into from Jun 22, 2017

Conversation

@kalikiana
Copy link
Contributor

@kalikiana kalikiana commented Jun 2, 2017

Support doing snapcraft snap --target-arch= with parts using the Rust plugin

@kalikiana kalikiana self-assigned this Jun 2, 2017
@sergiusens sergiusens changed the title Rust rust: Add support for cross-compilation rust plugin: Add support for cross-compilation Jun 7, 2017
@kalikiana kalikiana requested review from elopio and kyrofa Jun 12, 2017
Copy link
Contributor

@elopio elopio left a comment

Nice, nice :)
Please add a manual test. And please ping @ChrisMacNaughton to get his expert review on this.

@@ -91,6 +92,17 @@ def test_stage_rust_with_source_and_source_subdir(self):
os.path.join('parts', 'rust-subdir', 'src', 'subdir',
'Cargo.lock'), FileExists())

def test_cross_compiling(self):
if snapcraft.ProjectOptions().deb_arch != 'amd64':
self.skipTest('The test only handles amd64 to armhf')

This comment has been minimized.

@elopio

elopio Jun 14, 2017
Contributor

s/armhf/arm64


# Cf. http://doc.crates.io/config.html
with suppress(FileExistsError):
os.mkdir(os.path.join(self.builddir, '.cargo'))

This comment has been minimized.

@elopio

elopio Jun 14, 2017
Contributor

I prefer os.makedirs(..., exist_ok=True). Was it @kyrofa who prefered suppress? Any reason for that?

# Cf. http://doc.crates.io/config.html
with suppress(FileExistsError):
os.mkdir(os.path.join(self.builddir, '.cargo'))
with open(os.path.join(self.builddir, '.cargo', 'config'), 'w') as f:

This comment has been minimized.

@elopio

elopio Jun 14, 2017
Contributor

a nit, I would put the cargo_dir in a variable, so there's no need to do the os.path.join again.

@@ -93,6 +91,9 @@ def __init__(self, name, options, project):

def build(self):
super().build()

self._write_config()

This comment has been minimized.

@elopio

elopio Jun 14, 2017
Contributor

This is only for cross-compiling right? What do you think about naming it _write_crosscompile_config?

'--prefix={}'.format(self._rustpath),
'--disable-sudo', '--save'] + options
if self.project.is_cross_compiling:
cmd.append("--with-target={}".format(self._target))

This comment has been minimized.

@elopio

elopio Jun 14, 2017
Contributor

single quotes please.

'Expected {!r} to be in {!r}'.format(
self._expected_arch, arch))
except Exception as e:
raise ValueError('Unexpected magic {!r}'.format(magic)) from e

This comment has been minimized.

@elopio

elopio Jun 14, 2017
Contributor

I'm not understanding this exception. What does it mean to have an unexpected magic?

Copy link
Member

@kyrofa kyrofa left a comment

A few niggly things from me, but great job here! I agree that someone more qualified in Rust than us needs to take a look before this should land, though.

if snapcraft.ProjectOptions().deb_arch != 'amd64':
self.skipTest('The test only handles amd64 to arm64')

target_arch = 'arm64'

This comment has been minimized.

@kyrofa

kyrofa Jun 15, 2017
Member

Unless I'm missing something, this only seems to be used once. Why are we using a variable?

'ppc64el': 'powerpc64le-{}-{}',
}
self._target = rust_targets.get(self.project.deb_arch).format(
'unknown-linux', 'gnu')

This comment has been minimized.

@kyrofa

kyrofa Jun 15, 2017
Member

Is there a reason for the previous dict being all templates if this is the only option? Why not just add this into the list and do away with the format?

This comment has been minimized.

@kalikiana

kalikiana Jun 19, 2017
Author Contributor

  • Making every single target twice as long adds noise and makes it less easy to see what's relevant.
  • If in the future we care about musl we can replace it easily.
}
self._target = rust_targets.get(self.project.deb_arch).format(
'unknown-linux', 'gnu')
if not self._target:

This comment has been minimized.

@kyrofa

kyrofa Jun 15, 2017
Member

If there isn't an entry in the above dict for self.project.deb_arch, we'll never get here-- the use of format will cause 'NoneType' object has no attribute 'format'. If we get rid of the format this will work, though.

super().clean_build()

with suppress(FileExistsError):
shutil.rmtree(self._cargo_dir)

This comment has been minimized.

@kyrofa

kyrofa Jun 15, 2017
Member

Wait... why would rmtree raise FileExistsError? If a file is in use? That sounds like a situation where we shouldn't suppress. Curious what's happening here.

try:
arch = magic.split(',')[1]
except IndexError as e:
raise ValueError('Failed to parse magic {!r}'.format(magic)) from e

This comment has been minimized.

@kyrofa

kyrofa Jun 15, 2017
Member

Thank you for this, very nice.

@kyrofa kyrofa added this to the 2.32 milestone Jun 17, 2017
@ChrisMacNaughton
Copy link
Contributor

@ChrisMacNaughton ChrisMacNaughton commented Jun 20, 2017

Been watching this one, code looks OK but I don't have any ARM hardware to test on ;-)

@elopio
Copy link
Contributor

@elopio elopio commented Jun 20, 2017

Thanks @ChrisMacNaughton!
@cholcombe973 would you like to give it a look too?

We want to get more expert knowledge into our plugins, so if the two of you could help us with the rust plugin it would be great.

Copy link

@cholcombe973 cholcombe973 left a comment

Overall I think it looks good. Nicely done :)
I have some small questions like why gcc, binutils, libc6-dev was removed.

self.project_options)
os.makedirs(plugin.sourcedir)

plugin.enable_cross_compilation()

This comment has been minimized.

@cholcombe973

cholcombe973 Jun 20, 2017

This needs to catch NotImplementedError

This comment has been minimized.

@kalikiana

kalikiana Jun 21, 2017
Author Contributor

Good catch (no pun intended). I added a separate test for that.


1. Go to integration_tests/snaps/rust-hello.
2. Run `snapcraft snap --target-arch=armhf`.
3. Copy the snap to a Raspberry Pi.

This comment has been minimized.

@cholcombe973

cholcombe973 Jun 20, 2017

We could also probably help users without a Raspberry Pi test with qemu instructions.

This comment has been minimized.

@kalikiana

kalikiana Jun 21, 2017
Author Contributor

I was talking about this with @elopio a couple of weeks ago. We could consider an integration test using qemu. But we can't run the test snap on a native armhf machine. So the manual test is specifically for this.

This comment has been minimized.

@cholcombe973

cholcombe973 Jun 21, 2017

I see. Ok I was just wondering

@@ -76,9 +77,6 @@ def get_build_properties(cls):
def __init__(self, name, options, project):
super().__init__(name, options, project)
self.build_packages.extend([

This comment has been minimized.

@cholcombe973

cholcombe973 Jun 20, 2017

I think we need the C cross toolchain here and the cross compiled std crates no?
Also I think @ChrisMacNaughton did you need these libs for the alacrity snap you did awhile back?

This comment has been minimized.

@kalikiana

kalikiana Jun 21, 2017
Author Contributor

I attempted to removed redundant packages here. Unfortunately our integration tests are not 100% trustworthy because gcc is implicitly present on CI so I verified this again in a LXD container and gcc is indeed needed, the other packages aren't - at least for the test packages in snapcraft, if there's others we need to add new tests.
rustup pulls in the target-specific toolchain, see the cargo config that's created for that purpose.
gcc, libc and friends for cross-compiling are added by snapcraft - the rust plugin doesn't need to do it.

@kyrofa
kyrofa approved these changes Jun 22, 2017
Copy link
Member

@kyrofa kyrofa left a comment

After the latest changes, looks good to me!

@elopio elopio merged commit 94fb106 into snapcore:master Jun 22, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
kalikiana added a commit to kalikiana/snapcraft that referenced this pull request Aug 3, 2017
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

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