Hi @kiashok. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Project checks aren't happy, seems there's differing dependency versions between go.mod at the root and the integration/client/go.mod

Project checks aren't happy, seems there's differing dependency versions between go.mod at the root and the integration/client/go.mod

Yeah, I saw that. The latest commit fixes the project check failures

Project checks aren't happy, seems there's differing dependency versions between go.mod at the root and the integration/client/go.mod

Yeah, I saw that. The latest commit fixes the project check failures

All checks have passed

@dmcgowan Hi Derek, would you be able to take a look please?

Go will embed the version information

I am not sure I understand the question. Are you asking if we need github.com/josephspurrier/goversioninfo v1.4.0 ?

Go will embed the version information

I am not sure I understand the question. Are you asking if we need github.com/josephspurrier/goversioninfo v1.4.0 ?

Yes. Does this need be be vendored here.

@helsaawy ping.. just double checking if the import _ is/was necessary here... E.g. to avoid using generated external syso? Just skimming the newly added vendor .. thought I should ask if this is on purpose / needed.

@helsaawy ping.. just double checking if the import _ is/was necessary here... E.g. to avoid using generated external syso? Just skimming the newly added vendor .. thought I should ask if this is on purpose / needed.

@mikebrow Thats weird that its being vendored in here, since the import in tools.go is under a //go:build tools constraint...
I would have imagined that it would be ignored since its not used

But yes, we use github.com/josephspurrier/goversioninfo/cmd/goversioninfo to generate the syso files via //go:generate directives: https://github.com/microsoft/hcsshim/blob/main/cmd/containerd-shim-runhcs-v1/main.go#L29

Its _ imported following this advice on how to track the version of tools needed for the repo, so we can always use the correct version.

Sorry--another mod update was merged and you need to resolve merge conflicts.

Minor nit: your changelog in your opening PR comment shows rc.2->rc.3, but we currently are at rc.1; the correct changelog is quite a bit bigger and includes the goversioninfo change that is discussed above (I was curious to see the change that brought in the dependency, and initially couldn't find it because it is not in rc.2->rc.3)

@helsaawy ping.. just double checking if the import _ is/was necessary here... E.g. to avoid using generated external syso? Just skimming the newly added vendor .. thought I should ask if this is on purpose / needed.

@mikebrow Thats weird that its being vendored in here, since the import in tools.go is under a //go:build tools constraint... I would have imagined that it would be ignored since its not used

But yes, we use github.com/josephspurrier/goversioninfo/cmd/goversioninfo to generate the syso files via //go:generate directives: https://github.com/microsoft/hcsshim/blob/main/cmd/containerd-shim-runhcs-v1/main.go#L29

Its _ imported following this advice on how to track the version of tools needed for the repo, so we can always use the correct version.

nod.. I think the key element in that guidance is.. and "want to ensure that everyone is using the same version of that tool"

Here we have a build only dependency for windows platform shim propagating up that is only used in build? If some other binary in the containerd echo system should be/needs to be using the same version makes sense. If that is not the case probably should remove it. I was looking for some constant struct that was overloaded at build, but since it's just external syso... Thing that caught my eye at first is it seems to be a private/personal repo, also we are always trying to remove extra vendor dependencies where it makes sense.

IMO ... open an issue to address in the shim repo, and merge this PR (with phil's change request to the description, and rebase w/conflict resolution, and will get the update later :-)

Sorry--another mod update was merged and you need to resolve merge conflicts.

Minor nit: your changelog in your opening PR comment shows rc.2->rc.3, but we currently are at rc.1; the correct changelog is quite a bit bigger and includes the goversioninfo change that is discussed above (I was curious to see the change that brought in the dependency, and initially couldn't find it because it is not in rc.2->rc.3)

Just resolved merge conflicts and also updated the description

@helsaawy ping.. just double checking if the import _ is/was necessary here... E.g. to avoid using generated external syso? Just skimming the newly added vendor .. thought I should ask if this is on purpose / needed.

@mikebrow Thats weird that its being vendored in here, since the import in tools.go is under a //go:build tools constraint... I would have imagined that it would be ignored since its not used
But yes, we use github.com/josephspurrier/goversioninfo/cmd/goversioninfo to generate the syso files via //go:generate directives: https://github.com/microsoft/hcsshim/blob/main/cmd/containerd-shim-runhcs-v1/main.go#L29
Its _ imported following this advice on how to track the version of tools needed for the repo, so we can always use the correct version.

nod.. I think the key element in that guidance is.. and "want to ensure that everyone is using the same version of that tool"

Here we have a build only dependency for windows platform shim propagating up that is only used in build? If some other binary in the containerd echo system should be/needs to be using the same version makes sense. If that is not the case probably should remove it. I was looking for some constant struct that was overloaded at build, but since it's just external syso... Thing that caught my eye at first is it seems to be a private/personal repo, also we are always trying to remove extra vendor dependencies where it makes sense.

IMO ... open an issue to address in the shim repo, and merge this PR (with phil's change request to the description, and rebase w/conflict resolution, and will get the update later :-)

Updated the PR after resolving conflicts + changing description to include change list between v0.10.0-rc.1 and v0.10.0-rc3

@mikebrow @dmcgowan Could you please help merge this change? Or do we need more sign offs?

@mikebrow @dmcgowan Could you please help merge this change? Or do we need more sign offs?

ping on this!

@kiashok @helsaawy How about putting tools.go in a separate module or use go install? I still don't want to have goversioninfo.

@kiashok @helsaawy How about putting tools.go in a separate module or use go install? I still don't want to have goversioninfo.

@helsaawy worked on this. I cut a new shim tag v0.10.0-rc.4 and updated this PR.

@kiashok @helsaawy How about putting tools.go in a separate module or use go install? I still don't want to have goversioninfo.

@helsaawy worked on this. I cut a new shim tag v0.10.0-rc.4 and updated this PR.

@dmcgowan @mikebrow could you please review the new changes made? This shim update includes some important fixes in hccshim and would be great if we can make it into containerd/1.7. Thanks!

Concerns about the goversioninfo addition have been resolved in this latest update. Thanks!