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

Migrate clippy and rustfmt tests to be more friendly and remove worka… #6222

Merged
merged 1 commit into from Feb 28, 2019

Conversation

Projects
None yet
5 participants
@scotthain
Copy link
Contributor

scotthain commented Feb 26, 2019

…rounds for container issues.

Signed-off-by: Scott Hain shain@chef.io

@thesentinels

This comment has been minimized.

Copy link
Contributor

thesentinels commented Feb 26, 2019

Thanks for the pull request! Here is what will happen next:

  1. Your PR will be reviewed by the maintainers
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

add_lints_to_clippy_args -A "${unexamined_lints[@]+"${unexamined_lints[@]}"}"
add_lints_to_clippy_args -A "${allowed_lints[@]+"${allowed_lints[@]}"}"
add_lints_to_clippy_args -W "${lints_to_fix[@]+"${lints_to_fix[@]}"}"
add_lints_to_clippy_args -D "${denied_lints[@]+"${denied_lints[@]}"}"

This comment has been minimized.

Copy link
@christophermaier

christophermaier Feb 26, 2019

Contributor

What do these changes do?

This comment has been minimized.

Copy link
@scotthain

scotthain Feb 26, 2019

Author Contributor

Allows for empty arrays. I'm totally open to other possibilities, but this allows us to essentially pass an empty string in the event that there is a 0-element array, which throws an unused error.

This comment has been minimized.

Copy link
@baumanj

baumanj Feb 26, 2019

Member

That shouldn't be necessary. add_lints_to_clippy_args modifies an array clippy_args, not a string.

I just tried it with unexamined_lints as an empty array and it seems to work. Can you show me how you're getting an error?

This comment has been minimized.

Copy link
@scotthain

scotthain Feb 26, 2019

Author Contributor

This is the error I was looking to avoid: https://buildkite.com/chef/habitat-sh-habitat-master-verify/builds/581#a7a0d244-034a-4097-84f2-008562c13102

info: downloading component 'clippy'
  | info: installing component 'clippy'
  | test/run_clippy.sh: line 122: unexamined_lints[@]: unbound variable
  | make: *** [lint] Error 1

This comment has been minimized.

Copy link
@baumanj

baumanj Feb 27, 2019

Member

That's weird. Is this the exact version of run_clippy.sh that was used in that run? In that version, unexamined_lints isn't unbound, it's initialized to an empty array on line 39.

I try this locally and it's not a problem:

➤ bash -l
$ set -eou pipefail
$ unexamined_lints=()
$ clippy_args=()
$
$ add_lints_to_clippy_args() {
>   flag=$1
>   shift
>   for lint
>   do
>     clippy_args+=("$flag" "${lint}")
>   done
> }
$ echo "${unexamined_lints[@]}"

$ add_lints_to_clippy_args -A "${unexamined_lints[@]}"
$ echo $?
0
@@ -2,6 +2,10 @@

set -eou pipefail

source ./support/ci/shared.sh

This comment has been minimized.

Copy link
@christophermaier

christophermaier Feb 26, 2019

Contributor

Is there a distinction to be made between what goes into support/ci and .buildkite?

This comment has been minimized.

Copy link
@scotthain

scotthain Feb 26, 2019

Author Contributor

This is something we'll be revisiting - right now things are spread between support/ci, .buildkite, test, etc. I didn't want to tackle that in this PR since it's going to require a bit more juggling.

This comment has been minimized.

Copy link
@christophermaier

christophermaier Feb 27, 2019

Contributor

Can we get an issue to track that work? I doubt you'll forget about it, since I'm sure this is pretty near the top of your mind right now, but it'd be good for general board-walking and awareness purposes.

This comment has been minimized.

Copy link
@scotthain

scotthain Feb 27, 2019

Author Contributor

Done! #6227

if sudo -E $rustup component add --toolchain "$toolchain" rustfmt; then
cargo_fmt="$rustup run $toolchain cargo fmt --all -- --check"
if rustup component add --toolchain "$toolchain" rustfmt; then
cargo_fmt="rustup run $toolchain cargo fmt --all -- --check"

This comment has been minimized.

Copy link
@christophermaier

christophermaier Feb 26, 2019

Contributor

You and @raskchanky should coordinate: see #6217

This comment has been minimized.

Copy link
@scotthain

scotthain Feb 26, 2019

Author Contributor

Oh very cool - we should probably combine and shove all that into shared.sh - what do you think @raskchanky ?

This comment has been minimized.

Copy link
@scotthain

scotthain Feb 26, 2019

Author Contributor

I almost did this but now I'm glad I didn't. What do you think about essentially making each of these things functions in shared.sh:

  • install rustup (if not already installed)
  • install rust toolchain (if not already installed)
  • install rust component (if not already installed)
    and have sane defaults and whatnot.

I think then we could then keep a install-latest|last-nightly-rust in the rustfmt script based on install-nightly-rustfmt but all it would do is call the appropriate functions above.

Or better yet - if there's a way to query for the latest rustfmt from an external place and target the right nightly.

This comment has been minimized.

Copy link
@raskchanky

raskchanky Feb 26, 2019

Member

@scotthain Sure, that sounds fine. The use case for my PR was "oh, my PR is failing CI because I'm using the wrong nightly rust. It'd be nice to just run a script and have all that taken care of for me, rather than having to do all the manual steps myself". Your function idea sounds like it would work. If you go that route, I'll close my PR and open a new one after yours merges that just leverages the functions to do the right thing.

@baumanj
Copy link
Member

baumanj left a comment

In addition to the inline comments, I think there's just a bit more that needs to be added that would achieve two goals I have for friendly local usability:

  1. No network required if all the components are already installed
  2. No sudo unless necessary

Add this to shared.sh:

install_hab_pkg() {
  for ident; do
    installed_pkgs=$(hab pkg list "$ident")
    if [[ -z $installed_pkgs ]]; then
      sudo hab pkg install "$ident"
    else
      echo "$ident already installed"
    fi
  done
}

And replace the sudo hab pkg install commands in run_clippy.sh with:

install_hab_pkg core/bzip2 core/libarchive core/libsodium core/openssl core/xz core/zeromq
hab pkg binlink core/protobuf
Show resolved Hide resolved support/ci/rustfmt.sh Outdated
Show resolved Hide resolved support/ci/shared.sh Outdated
Show resolved Hide resolved test/run_clippy.sh Outdated
add_lints_to_clippy_args -A "${allowed_lints[@]}"
add_lints_to_clippy_args -W "${lints_to_fix[@]}"
add_lints_to_clippy_args -D "${denied_lints[@]}"
add_lints_to_clippy_args -A "${unexamined_lints[@]+"${unexamined_lints[@]}"}"

This comment has been minimized.

Copy link
@baumanj

baumanj Feb 26, 2019

Member

I don't understand this change

toolchain="${1:-stable}"

echo "--- :rust: Installing rustup"
curl https://sh.rustup.rs -sSf | sh -s -- --no-modify-path -y

This comment has been minimized.

Copy link
@baumanj

baumanj Feb 26, 2019

Member

It'd be nice to avoid the network call if we already have rustup and cargo:

  if command -v rustup && command -v cargo; then
    echo "--- :rust: $(rustup --version) already installed"
	else
    echo "--- :rust: Installing rustup"
    curl https://sh.rustup.rs -sSf | sh -s -- --no-modify-path -y
    source "$HOME"/.cargo/env
  fi
curl https://sh.rustup.rs -sSf | sh -s -- --no-modify-path -y
source "$HOME"/.cargo/env
echo "--- :rust: Installing rust $toolchain"
rustup toolchain install "$toolchain"

This comment has been minimized.

Copy link
@baumanj

baumanj Feb 26, 2019

Member

Similarly:

  if rustup component list --toolchain "$toolchain" &>/dev/null; then
    echo "--- :rust: rust $toolchain already installed"
  else
    echo "--- :rust: Installing rust $toolchain"
    rustup toolchain install "$toolchain"
  fi

This condition is a bit of an odd way to check for a particular toolchain being installed, but I don't know of a better way to leverage rustup's shorthand of stable or nightly.

@scotthain

This comment has been minimized.

Copy link
Contributor Author

scotthain commented Feb 27, 2019

@baumanj I think there's an ideological difference that we're having here, in regard to the use of sudo and the network. These scripts are intended to ensure the latest version of the hab packages and rust toolchain/components are installed. When running in CI, we attempt to ensure this by always installing fresh, rather than deferring to an existing installation. I'm not sure I understand the aversion to network usage in this case. (This could be ignorance on my part regarding how rustup/cargo operates, does it upgrade in place when installing things or is that a manual process?). With regards to sudo - I'd rather make sure we're installing the latest and I'm not sure what we gain by moving the sudo commands out of the script.

@baumanj

This comment has been minimized.

Copy link
Member

baumanj commented Feb 27, 2019

@baumanj I think there's an ideological difference that we're having here,

I'm not sure I understand the aversion to network usage in this case.

I think it's a CI versus local-dev issue. I sometimes work in places where I may have poor or no network access such as trains and planes, so doing a network operation when it's not necessary can result in a productivity blocker. The sudo aversion is more about generally trying to avoid privilege escalation, but it's sort of a sub-concern since the only place sudo is needed is to install things that require a network connection.

With regards to sudo - I'd rather make sure we're installing the latest and I'm not sure what we gain by moving the sudo commands out of the script.

Maybe this is my ignorance about the way our CI works. Do we always have a pristine environment to start with? If so, all the installs should be strictly necessary and will always get the latest version. For local development, when the build dependencies are already installed (and they pretty much always are, because I'm doing local development), I really do want to save a few seconds and/or eliminate the requirement of a good network connection to work on code that's all local to my machine already.

I'm open to different approaches to resolve the concerns. I think we can both get our needs met, and I think we agree that we should be running the same code locally and in CI to as great an extent as possible.

Migrate clippy and rustfmt tests to be more friendly and remove worka…
…rounds for container issues.

Signed-off-by: Scott Hain <shain@chef.io>

@scotthain scotthain force-pushed the shain/container_fix branch from 94c024b to 46bbae6 Feb 27, 2019

@scotthain

This comment has been minimized.

Copy link
Contributor Author

scotthain commented Feb 27, 2019

@baumanj et all - after discussing with @raskchanky - we're going to get this shipshape and merged and @raskchanky will add the functionality described here #6222 (comment) in a followup PR that will also include the "find the last nightly + rustfmt combo" code. I think that will bring the local UX to an acceptable level.

@raskchanky raskchanky merged commit 3d8cdff into master Feb 28, 2019

5 checks passed

DCO This commit has a DCO Signed-off-by line
buildkite/habitat-sh-habitat-master-verify Build #606 passed (1 hour, 1 minute, 30 seconds)
Details
buildkite/habitat-sh-habitat-master-website Build #75 passed (59 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details

@raskchanky raskchanky deleted the shain/container_fix branch Feb 28, 2019

chef-ci added a commit that referenced this pull request Feb 28, 2019

Update CHANGELOG.md with details from pull request #6222
Obvious fix; these changes are the result of automation not creative thinking.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.