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

Install a fixed version of rust for CI integration tests #39

Merged
merged 12 commits into from May 1, 2019

Conversation

matchai
Copy link
Member

@matchai matchai commented Apr 30, 2019

Description

Make rustup install a specific version of Rust along with the required Rust toolchain to run the build and tests.

  • Split out the Rust installation logic from within azure-pipelines.yml
  • Define a fixed version to be installed within Azure Pipelines
  • Run tests with rustup run using the required toolkit
  • Make the same changes in the Dockerfile

Motivation and Context

In order to be able to have integration tests that run rust --version, we need the locally used Rust to be a fixed version, while running tests and builds with the current stable/beta/nightly.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Maintenance

Screenshots (if appropriate):

How Has This Been Tested?

  • I have tested using MacOS
  • I have tested using Linux
  • I have tested using Windows

Checklist:

  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.

@matchai matchai requested a review from a team April 30, 2019 01:40
@matchai
Copy link
Member Author

matchai commented Apr 30, 2019

@Multimo Do you think I'm overcomplicating this, or do you think this complexity makes sense?

@matchai matchai marked this pull request as ready for review April 30, 2019 01:48
Copy link
Contributor

@Multimo Multimo left a comment

Choose a reason for hiding this comment

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

Makes sense what your doing. My naive knee jerk reaction is to say the version being fix in the docker images is enough and you should not have to use rustup but i dont think i have the full context here.

@@ -1,4 +1,4 @@
FROM rust:latest
FROM rust:1.34.0
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense. :latest will always grab the latest version and would invalidate the tests so this is legit 👍

@@ -24,7 +24,8 @@ RUN mkdir benches
RUN touch benches/my_benchmark.rs

# This is a dummy build to get dependencies cached
RUN cargo build --release
RUN rustup install stable
RUN rustup run stable cargo build --release
Copy link
Contributor

Choose a reason for hiding this comment

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

I am honestly not sure about the significance of this? Could you expand a little why you made this change? Wouldn't the version of rust specified in the docker image default to stable?

@matchai matchai changed the title Install fixed version of rust for CI integration tests Install a fixed version of rust for CI integration tests May 1, 2019
@matchai matchai merged commit d945b03 into master May 1, 2019
@matchai matchai deleted the clean-up-pipelines branch May 1, 2019 15:12
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

3 participants