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

Should the overrides prefixes be matching on filesystem case, or Go package name? #47

Closed
TBBle opened this issue Sep 24, 2021 · 3 comments · Fixed by #51
Closed

Should the overrides prefixes be matching on filesystem case, or Go package name? #47

TBBle opened this issue Sep 24, 2021 · 3 comments · Fixed by #51
Assignees

Comments

@TBBle
Copy link

TBBle commented Sep 24, 2021

While trying to regenerate protobuf in hcsshim, I hit a surprising behaviour.

I was running this on Windows, and

protobuild github.com/Microsoft/hcsshim/internal/shimdiag

was not honouring the existing override in Protobuf.toml:

[[overrides]]
prefixes = ["github.com/Microsoft/hcsshim/internal/shimdiag"]
plugins = ["ttrpc"]

After much poking about, I found this works:

[[overrides]]
prefixes = [
  "github.com/Microsoft/hcsshim/internal/shimdiag",
  "github.com\\Microsoft\\hcsshim\\internal\\shimdiag",
]
plugins = ["ttrpc"]

Is this expected behaviour? I had expected (from my very limited exposure to this tool) that all these paths were in "Go package path" format, rather than being filesystem paths.

I only worked out what was going on when I noticed that the prefix being looked up is defined with filepath.Rel, and perhaps filepath.ToSlash should be used on the result before further processing? Or perhaps that would also be surprising for packages defined on the filesystem. (I don't know if Go enforces /-separators in that case or not.)

This is with protobuild 6b023c6, rather than 0.1.0, due to #46.

@stevvooe
Copy link
Member

This is not expected. It should handle the paths identically on both systems.

@kzys Can you have a look at this?

@kzys
Copy link
Member

kzys commented Feb 18, 2022

Sure.

@kzys kzys self-assigned this Feb 18, 2022
@kzys
Copy link
Member

kzys commented Feb 21, 2022

I only worked out what was going on when I noticed that the prefix being looked up is defined with filepath.Rel, and perhaps filepath.ToSlash should be used on the result before further processing?

@TBBle is right. Protobuild is assuming that what Go returns as filepaths are compatible with Go's import paths, which is not true on Windows.

kzys added a commit to kzys/protobuild that referenced this issue Feb 21, 2022
Protobuild was assuming that canonicalized relative file paths and
Go import paths are the same. This is not true on Windows where
the former uses `\` but the latter uses (as like other OSes) `/`.

This change fixes the issue and adds some tests around the
implementation to prevent regressions.

Fixes containerd#47.

Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
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.

3 participants