Use osversion.Build() utility, and add a sync.Once #996
Conversation
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
| @@ -145,11 +145,11 @@ func verifyOptions(ctx context.Context, options interface{}) error { | |||
| return errors.New("PreferredRootFSTypeVHD requires at least one VPMem device") | |||
| } | |||
| } | |||
| if opts.KernelDirect && osversion.Get().Build < 18286 { | |||
| if opts.KernelDirect && osversion.Build() < 18286 { | |||
There was a problem hiding this comment.
Question: is there much overhead in the syscall that's made? (wondering if there would be benefits in setting a package-level variable to store the version in to prevent calling this repeatedly in some packages, or if that would not give (significant) benefits)
func GetVersion() (ver uint32, err error) {
r0, _, e1 := syscall.Syscall(procGetVersion.Addr(), 0, 0, 0, 0)
ver = uint32(r0)
if ver == 0 {
err = errnoErr(e1)
}
return
}
There was a problem hiding this comment.
I'll have to trial the overhead but one syscall will definitely be better than many 😄. You'd think we could just do this in an init, assign it to an exported global and be done with it.. Not sure if I'm missing something/some history on why we just call it over and over.
There was a problem hiding this comment.
@kevpar Should we move test/functional/manifest top level (or just out of test really), call Build() once in the osversion package and cache the result in an exported var and just use this everywhere? I don't have the history on why the manifest lives in test at the moment.
There was a problem hiding this comment.
The alternative could be to add a sync.Once in osversion.Get(); that way we don't have to add some "magic variable", and any consumer of that utility won't have to worry about it, and get the improvement "out of the box".
There was a problem hiding this comment.
That's what I have at the moment after firing up a new branch, I like it better
There was a problem hiding this comment.
Actually went ahead and added that as an extra commit in this branch (can remove if it's not needed)
There was a problem hiding this comment.
You're too quick for me 😎. Let's see what others think, I think it should be fine
There was a problem hiding this comment.
Switching to a global and sync.Once looks fine to me. I might be missing why moving manifest came up, though?
There was a problem hiding this comment.
No functional reason other than it feels weird to import this thing from the test dir when needed
There was a problem hiding this comment.
Ah, I thought it was related to this somehow. I'm in favor of moving it, but as a separate PR.
Given that it's _very_ unlikely that the Windows version changes at runtime, we can use a sync.Once to not repeatedly call windows.GetVersion() Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
changed the title
This PR updates our internal ADO repo to the github commit [d9474d2](microsoft@d9474d2). Related work items: microsoft#964, microsoft#965, microsoft#966, microsoft#967, microsoft#968, microsoft#969, microsoft#970, microsoft#971, microsoft#972, microsoft#974, microsoft#975, microsoft#976, microsoft#977, microsoft#978, microsoft#979, microsoft#980, microsoft#981, microsoft#982, microsoft#983, microsoft#984, microsoft#987, microsoft#990, microsoft#991, microsoft#992, microsoft#993, microsoft#995, microsoft#996, microsoft#997, microsoft#1000
When updating #556, I noticed other uses of this, so thought I'd do a minor refactor.
In this PR I noticed there's some intermediate build numbers used that don't (yet) have a corresponding
constin osversion;V19H1?V20H1?V20H1?V20H2?)Giving that the "intermediate" versions have now made their way in releases, I assume (famous last words) that those can now be replaced with
<first release that contains this change>; happy to make those changes (and/or add additionalconstin the package)