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

Maybe: restore-only/no-save/save-if option #66

Closed
CAD97 opened this issue Jul 22, 2022 · 1 comment
Closed

Maybe: restore-only/no-save/save-if option #66

CAD97 opened this issue Jul 22, 2022 · 1 comment

Comments

@CAD97
Copy link

CAD97 commented Jul 22, 2022

Use case: something like

jobs:

  # Run tests in various configurations.
  test:
    name: cargo test
    strategy:
      fail-fast: false
      matrix:
        os: [ubuntu-latest, macos-latest, windows-latest]
        features: [--no-default-features, "", --all-features]
    runs-on: ${{ matrix.os }}
    steps:
      - name: Checkout repository
        uses: actions/checkout@v3
      - name: Install Rust
        uses: ./.github/actions/rustup
      - name: Cache Rust
        uses: Swatinem/rust-cache@v2
        with:
          save-if: ${{ matrix.features == "--all-features" }}
      - name: Build tests
        run: cargo test ${{ matrix.features }} --no-run
      - name: Run tests
        run: cargo test ${{ matrix.features }} --no-fail-fast

The assumption here is that --all-features is additive, and so contains a cache sufficient for --no-default-features and the default feature set. The save-if option allows the workflow to guarantee that the stored cache is the --all-features one, which should be sufficient for the other feature sets, and not racing differently populated caches against each other.

The alternative is to split the cache initialization into a separate job, something like

  # Create test build cache.
  test-cache:
    name: cargo test --no-run
    strategy:
      fail-fast: false
      matrix:
        os: [ubuntu-latest, macos-latest, windows-latest]
    runs-on: ${{ matrix.os }}
    steps:
      - name: Checkout repository
        uses: actions/checkout@v3
      - name: Install Rust
        uses: ./.github/actions/rustup
      - name: Cache Rust
        id: cache
        uses: Swatinem/rust-cache@v2
        with:
          shared-key: build
      - name: Build tests
        run: cargo test --all-features --all-targets --no-run
        if: ${{ !steps.cache.outputs.cache-hit }}

  # Run tests in various configurations.
  test:
    name: cargo test
    needs: test-cache
    strategy:
      fail-fast: false
      matrix:
        os: [ubuntu-latest, macos-latest, windows-latest]
        features: [--no-default-features, "", --all-features]
    runs-on: ${{ matrix.os }}
    steps:
      - name: Checkout repository
        uses: actions/checkout@v3
      - name: Install Rust
        uses: ./.github/actions/rustup
      - name: Cache Rust
        uses: Swatinem/rust-cache@v2
        with:
          shared-key: build
      - name: Build tests
        run: cargo test ${{ matrix.features }} --all-targets --no-run
      - name: Run tests
        run: cargo test ${{ matrix.features }} --all-targets --no-fail-fast

and while this will ensure that the initial cache population run will have all of the test matrix jobs restore from cache, it requires undesirable duplication of the tests job and thus

  • on initial cache-population run, builds local tests and discards that work; and
  • on subsequent fresh-cache runs, still has to initialize and restore from cache before starting test jobs.

(If feature selection of the local crate changes which dependencies are selected upstream, this cache sharing will be ineffective and a pessimization on runtime. Perhaps that's reason enough to not encourage it. But it still does reduce the amount of cache significantly over caching each featureset independently.)

Implementation-wise, this should just amount to an early-out in save.ts when !core.getBooleanInput("save-if").


More generally, this usecase would be well served by a jobs.<job-id>.continues-from: <job-id> (so long as it manages to respect a matrix-keyed runs-on) but that seems unlikely to get from upstream. (It would also require making sure that cache saving is idempotent and doesn't trash local state.)

@Swatinem
Copy link
Owner

That does sound like a great idea

lucasfernog added a commit to lucasfernog/rust-cache that referenced this issue Nov 6, 2022
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

No branches or pull requests

2 participants