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

Revisit the cpuType type definition #647

Closed
AkihiroSuda opened this issue Feb 14, 2022 · 4 comments · Fixed by #656
Closed

Revisit the cpuType type definition #647

AkihiroSuda opened this issue Feb 14, 2022 · 4 comments · Fixed by #656
Milestone

Comments

@AkihiroSuda
Copy link
Member

I already merged this PR, but on second thought we could define this as []struct{Type string, Arch Arch} so that a single YAML can be used on multiple archs?

Originally posted by @AkihiroSuda in #643 (comment)

@jandubois
Copy link
Member

I don't know. Initially I thought that it would rarely be useful, but I can see it for defaults.yaml, so you can override the cpu type defaults for the different architectures.

If we want to change it, we should do it before the next release, so there is no backwards compatibility issue!

@AkihiroSuda AkihiroSuda added this to the v0.8.3 milestone Feb 14, 2022
@jandubois
Copy link
Member

we could define this as []struct{Type string, Arch Arch} so that a single YAML can be used on multiple archs?

I'm going to work on this, but I think it should be a map[Arch]string instead of a slice. It makes no sense to specify multiple CPU types for the same arch. The defaults would be:

cpuType:
  aarch64: cortex-a72
  x86_64: qemu64

Let me know if I misunderstand!

@AkihiroSuda
Copy link
Member Author

AkihiroSuda commented Feb 17, 2022

map[Arch]string

I thought it is inconsistent with the images struct, but not a huge deal probably, so SGTM

@jandubois
Copy link
Member

I thought it is inconsistent with the images struct, but not a huge deal probably, so SGTM

For images it (used to) make sense because you could have a faster local location, and a slower download location for the same arch as a fallback. Now that we have built-in caching, this seems questionable, but probably not worth changing.

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 a pull request may close this issue.

2 participants