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

Let Rust plugin fetch dependencies in pull #908

Merged
merged 11 commits into from Dec 6, 2016
Merged

Let Rust plugin fetch dependencies in pull #908

merged 11 commits into from Dec 6, 2016

Conversation

ChrisMacNaughton
Copy link
Contributor

Pulling the dependencies in the pull stage allows
building Rust snaps on Launchpad builders,
which are offline during the build stage

Pulling the dependencies in the pull stage allows
building Rust snaps on Launchpad builders,
which are offline during the build stage
@sergiusens
Copy link
Collaborator

Thanks for the PR, seems the unit tests need updating though.
You can try locally by running

./runtests.sh unit

Also might as well run

./runtests.sh static

Before pushing again. Thanks again

@ChrisMacNaughton
Copy link
Contributor Author

It looks like the tests failed because travis couldn't update packages?

Copy link
Contributor

@come-maiz come-maiz left a comment

Choose a reason for hiding this comment

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

Thanks @ChrisMacNaughton
I think it would be nice to add an integration test with a simple rust project that has dependencies.
You can take a look at integration_tests/test_rust_plugin.py

@@ -86,7 +86,7 @@ def test_pull(self, run_mock, script_mock):

plugin.pull()

self.assertEqual(1, run_mock.call_count)
self.assertEqual(2, run_mock.call_count)
Copy link
Contributor

Choose a reason for hiding this comment

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

This test would require more than checking that rust was called twice. Please take a look at the next statement that checks the arguments of the first call, and do something similar to the second.

'build-essential',
'git',
'curl',
'file',
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know rust, so I will trust you here that these are needed. But just curious, is there a doc that mentions these requirements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the things that were required for me to get rustc installed on one of the launchpad builders. Can't find specific documentation about it but that's where this list came from.

@ChrisMacNaughton
Copy link
Contributor Author

@ElOpio tests should be accurate now, and the integration rust project now pulls in a dependency 😄

Copy link
Contributor

@come-maiz come-maiz left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@kyrofa kyrofa left a comment

Choose a reason for hiding this comment

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

A few suggestions on the build packages.

@@ -54,6 +54,12 @@ def schema(cls):

def __init__(self, name, options, project):
super().__init__(name, options, project)
self.build_packages.extend([
'build-essential',
'git',
Copy link
Contributor

Choose a reason for hiding this comment

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

Git should be pulled in simply by using a git source. It shouldn't be required here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless the pull of rust eventually turns into a git clone. Is that the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pulling Rust shouldn't clone, but using Cargo for the build can

@@ -54,6 +54,12 @@ def schema(cls):

def __init__(self, name, options, project):
super().__init__(name, options, project)
self.build_packages.extend([
'build-essential',
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a large meta package. Are you sure you need all of it?

Copy link
Contributor

@kyrofa kyrofa left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

Copy link
Collaborator

@sergiusens sergiusens left a comment

Choose a reason for hiding this comment

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

Thanks for this great functionality improvement on separating pull and build effectively in the plugin. I do however need to ask you to revert the change on the wiki downloader test as master has a proper (air quotes) fix for this (at least one where the endpoint won't change underneath us).

@@ -27,7 +27,8 @@ def test_downloader_with_wiki_parts(self):
snap_path = self.build_snap(self.snap_content_dir)
self.install_snap(snap_path, 'downloader', '1.0')
expected = (
'.*<title>The leading OS for PC, tablet, phone and cloud '
'.*<title>The leading operating system for PCs, tablets, '
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be reverted as it was fixed in master with a more permanent solution.

@ChrisMacNaughton
Copy link
Contributor Author

@sergiusens I've reverted that change, and it looks like that is the issue with the autopkgtests still.

@codecov-io
Copy link

Current coverage is 97.06% (diff: 100%)

Merging #908 into master will increase coverage by <.01%

@@             master       #908   diff @@
==========================================
  Files           161        161          
  Lines         16600      16603     +3   
  Methods           0          0          
  Messages          0          0          
  Branches       1244       1244          
==========================================
+ Hits          16113      16116     +3   
  Misses          302        302          
  Partials        185        185          

Powered by Codecov. Last update 97af16f...02f4e1f

@kyrofa kyrofa merged commit a87decf into snapcore:master Dec 6, 2016
kalikiana pushed a commit to kalikiana/snapcraft that referenced this pull request Apr 6, 2017
Pulling the dependencies in the pull stage allows
building Rust snaps on Launchpad builders,
which are offline during the build stage

LP: #1647847
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants