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

Add rustc_hash output #17

Closed
wants to merge 2 commits into from
Closed

Add rustc_hash output #17

wants to merge 2 commits into from

Conversation

Kijewski
Copy link

I.e. the a8314ef7d part in rustc 1.62.0 (a8314ef7d 2022-06-27).
This makes it possible to use the output e.g. as a cache key.
actions-rs/toolchain has the same output with the same function.

I.e. the `a8314ef7d` part in `rustc 1.62.0 (a8314ef7d 2022-06-27)`.
This makes it possible to use the output e.g. as a cache key.
`actions-rs/toolchain` has the same output with the same function.
Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Wouldn't version work just as well for a cache key?

@Kijewski
Copy link
Author

No, because version contains spaces, and cache keys are space separated, also.

I guess you could replace the spaces by underscores or dashes instead of extracting the git revision, but I don't think there would be much advantage in that.

@dtolnay
Copy link
Owner

dtolnay commented Jul 15, 2022

Ah okay. I have not used the actions/cache action but I am checking what you are doing in https://github.com/Kijewski/tzdb/blob/v0.2.7/.github/actions/setup-rust/action.yaml with whitespace separation in restore-keys.

From https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows#input-parameters-for-the-cache-action and the following code in actions/cache:

const restoreKeys = utils.getInputAsArray(Inputs.RestoreKeys);
export function getInputAsArray(
    name: string,
    options?: core.InputOptions
): string[] {
    return core
        .getInput(name, options)
        .split("\n")
        .map(s => s.trim())
        .filter(x => x !== "");
}

it sounds like the restore keys are separated by newline, so spaces might not be an issue. A better reason against ${{steps.toolchain-install.outputs.version}} for this is just that it's verbose to read in stdout of the action, along with the following:

Keys have a maximum length of 512 characters, and keys longer than the maximum length will cause the action to fail.

meaning that shorter (than rustc --version) would generally be better for forming part of a cache key, like you are doing.

I think my preference would be to expose something dedicated to the cache key use case rather than exposing the git commit hash that rustc was built from. Perhaps something like this to avoid any fiddly parsing:

$(rustc --version | sha256sum | head -c9)

Another observation: it may be more useful to include the date in the cache key, as opposed to the rustc repo hash. Rationale: look at https://github.com/Kijewski/tzdb/runs/7307104972:

Run actions/cache@v3
  with:
    path:
      ~/.cargo/registry/index/
      ~/.cargo/registry/cache/
      ~/.cargo/git/db/
      target/
    key:
      check-Linux-nightly-38b72154d-ba5a620a25fdb47156afec842d6c821f6d4e75085e970e92a6b5e3d10a6f191c
    restore-keys:
      check-Linux-nightly-38b72154d-
      check-Linux-nightly-

/usr/bin/tar --use-compress-program zstd -d -xf /home/runner/work/_temp/51d7cea1-713d-4ba1-bb18-a142fca7b823/cache.tzst -P -C /home/runner/work/tzdb/tzdb
Cache restored successfully
Cache restored from key: check-Linux-nightly-7b46aa594-8b742841c3337363f21f56f2914fdeb4cba421d49f608d42ee85ae149fe863db

We can tell from this that it loaded some cache older than the one that was requested, because 38b72154d and 7b46aa594 are not the same. How much older?— no idea. If the presence of the old cached target/ is relevant to reproducing some job failure, we would need to install a bunch of nightlies with rustup to hunt for that hash, or try to deduce it from the rust-lang/rust commit history or your repo's CI history. An output like 20220701 would tend to be more helpful to directly show which older toolchain's cache output got loaded.

* commit-hash: rustc's source revision long commit hash
* abbrev-hash: rustc's source revision short commit hash
* commit-date: rustc's source revision commit date
* release: rustc's version

The earliest compatible rustc release is 1.16.0 (2017-03-10). Earlier
versions did not implement `--verbose --version`s.
@Kijewski
Copy link
Author

Ah, I was quite sure that any whitespaces are used to split the input, but the code shows it does not.

Making rustc_hash return the short hash would make your action a drop-in replacement for actions-rs, but maybe that is not needed. Using the date in the cache key seems like a good idea! I added a commit that makes the other outputs of rustc --verbose --version available, so the user can decide whether to use the revision, the date or a combination thereof.

@dtolnay
Copy link
Owner

dtolnay commented Jul 15, 2022

I added a compact cache key output based on just the date and commit hash. Would this address your use case, or is there still a need to have commit-hash and abbrev-hash and commit-date and release as separate outputs?

cachekey:
description: A short hash of the rustc version, appropriate for use as a cache key. "20220627a831"
value: ${{steps.rustc-version.outputs.cachekey}}

DATE=$(rustc +${{inputs.toolchain}} --version --verbose | sed -ne 's/^commit-date: \(20[0-9][0-9]\)-\([01][0-9]\)-\([0-3][0-9]\)$/\1\2\3/p')
HASH=$(rustc +${{inputs.toolchain}} --version --verbose | sed -ne 's/^commit-hash: //p')
echo "::set-output name=cachekey::$(echo $DATE$HASH | head -c12)"

@Kijewski
Copy link
Author

Thank you! The cachekey output works perfectly fine for me.

I only added the other outputs, because, well, the values exists. I don't have a use for the other outputs, and if someone does, I guess they will open an issue.

@Kijewski Kijewski closed this Jul 17, 2022
@Kijewski Kijewski deleted the pr-rustc_hash branch July 17, 2022 02:16
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.

2 participants