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

Jump to bottom

rust plugin: fix cargo builds and run tests #2158

Closed
wants to merge 3 commits into from
Closed

rust plugin: fix cargo builds and run tests #2158

wants to merge 3 commits into from

Conversation

tismith
Copy link

@tismith tismith commented Jun 7, 2018

This patch changes it so that we run all instances of cargo using the build environment. Fixes cases where a snap cleanbuild will error out with rustc being unable to be found during the pull step.

This is the issue I asked for help with on the forum: https://forum.snapcraft.io/t/problem-building-rust-based-snap/5776

Also adds 'cargo test' to the build step, to fail the build if the tests aren't passing.

Rust plugin unittests have been updated.

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • If this is a bugfix. Have you checked that there is a bug report open for the issue you are trying to fix on bug reports?
  • If this is a new feature. Have you discussed the design on the forum?
  • Have you successfully run ./runtests.sh static?
  • Have you successfully run ./runtests.sh unit?

@sergiusens
Copy link
Collaborator

thanks for the contribution! At first glance this looks good. I have re-triggered travis to take into account the timeout.
I do have a question though, is the test target a conventional target (as it would be for makefiles) or built in to cargo itself? If the latter and the fallbacks are good when no tests are available, this should be good as is.

@tismith
Copy link
Author

tismith commented Jun 7, 2018

is the test target a conventional target (as it would be for makefiles) or built in to cargo itself? If the latter and the fallbacks are good when no tests are available, this should be good as is.

Yes, cargo test is a builtin part of cargo, and will pass if no tests are available. E.g. here's the output from a basically empty crate (i.e. just after running cargo init --name testy in an empty directory):

$ cargo test
   Compiling testy v0.1.0 (file:///Users/tobysmith/src/home/rust/test)
    Finished dev [unoptimized + debuginfo] target(s) in 3.4 secs
     Running target/debug/deps/testy-2006db7acae3b59a

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
$ echo $?
0

@codecov-io
Copy link

codecov-io commented Jun 18, 2018 •
edited

Codecov Report

❗ No coverage uploaded for pull request base (master@13881c3). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2158   +/-   ##
=========================================
  Coverage          ?   91.25%           
=========================================
  Files             ?      196           
  Lines             ?    12454           
  Branches          ?     1846           
=========================================
  Hits              ?    11365           
  Misses            ?      734           
  Partials          ?      355
Impacted Files Coverage Δ
snapcraft/plugins/rust.py 94.64% <100%> (ø)

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 13881c3...9a16a9d. Read the comment docs.

@tismith
Copy link
Author

tismith commented Jun 19, 2018

Looks like 'cargo test' doesn't play nice with cross compiling. I'll rework the patch to not run the tests when cross compiling.

@tismith
rust plugin: fix cargo builds and run tests
b44f103 
This patch changes it so that we run all instances of cargo
using the build environment. Fixes cases where rustc can't
be found during the pull step.

Also adds 'cargo test' to the build step, to fail the build
if the tests aren't passing. We will only run the tests when we
aren't cross compiling.
@tismith
Copy link
Author

tismith commented Jun 24, 2018 •
edited

Looks like someone else has run into this same problem, and worked around it like I did with a custom rust plugin.

https://forum.snapcraft.io/t/snap-of-gutenberg/6080/3 and getzola/zola#322

@tismith tismith mentioned this pull request Jun 24, 2018
Closed
sergiusens
sergiusens reviewed Jun 25, 2018
View reviewed changes
snapcraft/plugins/rust.py
@@ -97,11 +97,24 @@ def __init__(self, name, options, project):
self._rustup = os.path.join(self._rustpath, "rustup.sh")
self._manifest = collections.OrderedDict()

def _test(self):
if self.project.is_cross_compiling:
Copy link
Collaborator

Choose a reason for hiding this comment

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

it would be nice to issue a warning here about skipping the test execution since it is cross compiling.

@sergiusens
Copy link
Collaborator

Travis seems to be confused

Cloning into 'snapcore/snapcraft'...
warning: Could not find remote branch rust-plugin-improvements to clone.
fatal: Remote branch rust-plugin-improvements not found in upstream origin

@sergiusens
Copy link
Collaborator

I have re-created the PR on #2170

@sergiusens sergiusens closed this Jun 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sergiusens sergiusens sergiusens approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tismith @sergiusens @codecov-io