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

Update cache-key with Runner.os and Arch #152

Merged
merged 2 commits into from Apr 29, 2022

Conversation

KKimj
Copy link
Contributor

@KKimj KKimj commented Apr 27, 2022

Hello..
This PR will be the last.

How about add ${{ runner.os }} on cache-key ?

Thanks.
Take Care!!

@kuhnroyal
Copy link
Contributor

This looks important :o Do we also need to include the architecture?

@KKimj
Copy link
Contributor Author

KKimj commented Apr 27, 2022

@kuhnroyal
Yeah, I think it would be better that way.
In fact, since there is no arm64 environment at actions, it may feel dubious.

@yurikoles
Copy link

Yes, there are no GitHub-hosted arm64 runners now, but one may use a self-hosted one, see #147.

@KKimj
Copy link
Contributor Author

KKimj commented Apr 27, 2022

@yurikoles
Oh.. Sorry.. I missed it.
Thanks you for pointing out it.
In that case, it would be reasonable to introduce arch.

@KKimj
Copy link
Contributor Author

KKimj commented Apr 27, 2022

How about like below?

${{ inputs.cache-key }}-${{ runner.os }}-${{ runner.arch }}-${{ inputs.channel }}-${{ inputs.flutter-version }}

@subosito
Copy link
Owner

@KKimj Yeah, I think that's better

@kuhnroyal
Copy link
Contributor

Use the ${{ inputs.architecture }}? It seems to have a sane default.

@KKimj
Copy link
Contributor Author

KKimj commented Apr 27, 2022

@kuhnroyal
I agree with you.
I think there is a conner case something like below

  • Run on M1, ARM64
  • Use x86-64 Flutter SDK, via Rosetta 2

Is it reasonable?
@subosito

@KKimj KKimj changed the title Add ${{ runner.os }} on cache-key Update cache-key with Runner.os and Arch Apr 27, 2022
@subosito
Copy link
Owner

I think it's ok, but it would be better if we could set the default by using the existing value given by GitHub runner, like RUNNER_ARCH or runner.arch if possible. What do you think, guys?

@KKimj
Copy link
Contributor Author

KKimj commented Apr 27, 2022

@subosito
It makes sense! Nevertheless, In my opinion, it looks better to maintain.

@KKimj
Copy link
Contributor Author

KKimj commented Apr 27, 2022

As It is an edge case, and without a self-hosting machine, there is no good way to test with github action.

@KKimj
Copy link
Contributor Author

KKimj commented Apr 29, 2022

@subosito
here we go!

@subosito subosito merged commit 0c3f142 into subosito:main Apr 29, 2022
@KKimj KKimj deleted the Add-RunnerOS-cache-key branch April 29, 2022 02:06
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

4 participants