actions-rs / meta

🦀 GitHub Actions for Rust - recipes, discussions, questions and ideas
https://github.com/actions-rs
Creative Commons Zero v1.0 Universal
353 stars 15 forks source link

[discussion] denser syntax for installing rustc + components #12

Closed yoshuawuyts closed 4 years ago

yoshuawuyts commented 4 years ago

Consider this job:

  check_fmt:
    name: Checking fmt
    runs-on: ubuntu-latest
    steps:
    - uses: actions/checkout@master

    - id: component
      uses: actions-rs/components-nightly@v1
      with:
        component: rustfmt

    - uses: actions-rs/toolchain@v1
      with:
          toolchain: ${{ steps.component.outputs.toolchain }}
          override: true

    - name: fmt
      run: cargo fmt --all -- --check

There's a subtle bug hiding here that takes a bit of reading to figure out. Namely, despite the fact that we're talking about rustfmt multiple times, we're never actually installing rustfmt. This requires an extra line of run: rustup component add rustfmt.

So I was thinking of ways in which this could be improved, and realized that currently we're still working with how we should do things, rather than declaring what we want. I would imagine a declarative syntax for the same of what we've done above to be:

  check_fmt:
    name: Checking fmt
    runs-on: ubuntu-latest
    steps:
    - uses: actions/checkout@master

    - uses: actions-rs/toolchain@v1
      with:
          toolchain: nightly
          components:
              - rustfmt

    - name: fmt
      run: cargo fmt --all -- --check

This would apply the automatic version fallback from components-nightly behind the scenes, but only for the components specified. And possibly even handle resolution for when there are multiple components that may need to be installed, ensuring that all of them exist on the version that's being installed (because selecting them individually could be unreliable when doing this manually; it's not common, but I've hit this at least once).

Hopefully this is a useful idea, and I mostly just wanted to open up a conversation on if we could perhaps simplify some of the workflows. Thanks heaps!

svartalf commented 4 years ago

Yes, that's what it should be in a future (I totally agree that declarative syntax looks so much better), but.. 2qj748

Consider the example of yours (I added clippy also to denote that we want multiple components at once):

    - uses: actions-rs/toolchain@v1
      with:
          toolchain: nightly
          components:
              - rustfmt
              - clippy

Main problem here is that there is no decent way exist to determine suitable nightly where multiple requested components are available:

  1. Components history page itself shows the last seven days only
  2. Per-component URLs (ex. for clippy) returns the most recent toolchain name, but there obviously will be a mismatches between different components

One possible solution would be a separate server, which will store the components history and will be able to find the toolchain based on the multiple components requested, but it seems like an unnecessary extra point of failure.

Lucky us, this rustup issue should solve this problem! As soon as it will be implemented, components-nightly Action will be automatically deprecated, because toolchain Action will be able to manage that by itself.

P.S. As an extra quirk, workflow syntax disallows YAML lists in the with: section, so it would be smth like that instead:

    - uses: actions-rs/toolchain@v1
      with:
          toolchain: nightly
          components: rustfmt, clippy
sdht0 commented 4 years ago

Hi. A new rustup is out with the relevant features: https://blog.rust-lang.org/2019/10/15/Rustup-1.20.0.html. Looking forward to using the new features in actions-rs :)

A related request is to add a recipe that shows how to setup a matrix of toolchains, ideally all 3: matrix: toolchain: [stable, beta, nightly]. The same commands should now work for all toolchains without needing to special case nightly.

svartalf commented 4 years ago

@sdht0 work and discussion on it goes at https://github.com/actions-rs/toolchain/pull/15

svartalf commented 4 years ago

https://github.com/actions-rs/toolchain/pull/19 was merged and published, so this issue will be closed now.

Original snippet by @yoshuawuyts can now be replaced with the following:

  check_fmt:
    name: Checking fmt
    runs-on: ubuntu-latest
    steps:
    - uses: actions/checkout@master
    - uses: actions-rs/toolchain@v1
      with:
          profile: minimal
          toolchain: nightly
          override: true
          components: rustfmt
    - name: fmt
      run: cargo fmt --all -- --check

Note that I also added profile: minimal, which is not required in that case, but reduces build time a little bit more.

@sdht0, it is now very easy to create a matrix build too, and as you suggested, I had created a recipe for that.

sdht0 commented 4 years ago

Nice! Thanks @svartalf.