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

Question on bin files without .exe suffix #131

Closed
ahmetb opened this issue Nov 7, 2018 · 15 comments · Fixed by #821
Closed

Question on bin files without .exe suffix #131

ahmetb opened this issue Nov 7, 2018 · 15 comments · Fixed by #821
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.

Comments

@ahmetb
Copy link
Member

ahmetb commented Nov 7, 2018

If a plugin's windows platform spec specifies something like bin: foo, we:

  1. copy the executable to plugin's installation dir as foo
  2. but we symlink it as kubectl-foo.exe on windows.

I am not at all sure that kubectl-foo.exe which points to foo (that has no .exe extension) can be executed. Can someone verify if this works or not?

Depending on that, (1) we need to make sure if a platform spec matches windows, its bin must end with ".exe" (2) we need to update existing plugin specs in krew-index.

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 26, 2019
@ahmetb
Copy link
Member Author

ahmetb commented Apr 26, 2019

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 26, 2019
@juanvallejo
Copy link

I think for windows, having a symlink that ends in ".exe" should be fine. We don't follow symlinks to check the original file's extension. This being said, I'll try to double check that this works on windows

@ahmetb
Copy link
Member Author

ahmetb commented Apr 30, 2019

Yeah the part I am not sure is whether execve equivalent of Windows actually runs files without an executable suffix, my guess would be not.

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 29, 2019
@ahmetb
Copy link
Member Author

ahmetb commented Jul 29, 2019

/remove-lifecycle stale
I plan on checking this + krew self-upgrade code-path on windows sometime.

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 29, 2019
@corneliusweig
Copy link
Contributor

corneliusweig commented Jul 31, 2019

/label good-first-issue
/label help-wanted

@ahmetb
Copy link
Member Author

ahmetb commented Jul 31, 2019

yeah the /label is failing us on default labels that aren't tracked by the test-infra here.

@ahmetb ahmetb added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Jul 31, 2019
@ahmetb
Copy link
Member Author

ahmetb commented Aug 19, 2019

Just had the time to verify this on windows:

The target of the symlink (the file it's pointing to) does not have to have an executable suffix (like .COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.PSC1).

So the value in the bin field doesn't have to have an .exe suffix, because our symlink in %HOME%.krew\bin will already have an .exe suffix.

That said, since we add .exe to every executable, if your plugin is actually not a win32 application, it will not work. For example, plugins that have a .ps1 or .bat extension currently don't work on Krew.

I'm thinking possibly we need to preserve the file extension based on the platform.bin field field on Windows.
/cc @corneliusweig

@corneliusweig
Copy link
Contributor

So to clarify, if a plugin is not a win32 application, we need to install a symlink with a correct file extension, such as .bat.

Do you happen to know a good way to determine the proper file extension for a given file on windows? Then this could easily be done without relying on plugin authors to do it correctly. I'm a bit worried that if we have to verify this manually, it will be difficult for plugin authors and reviewers alike to get it right.

@ahmetb
Copy link
Member Author

ahmetb commented Aug 20, 2019

The only clean solution that comes to my mind is that

  • bin field must contain a file extension if platform selector matches os:windows (enforce this only in cosmetic validation, not parsing, so that existing plugins continue to work, and any modified plugin spec must comply)

  • if GOOS=windows parse the installOperation.Platform.Spec.Bin, if empty, assume .exe (so that existing plugins continue to work) and modify createOrUpdateLink to accept `fileExt string parameter.

Thoughts?

@corneliusweig
Copy link
Contributor

It's not pretty, but it will do the job. Plugins with non-exe endings are probably rare exceptions anyway, in particular their architecture will be limited to just windows. So we can somewhat rely on the plugin authors to have verified that it works on windows machines.

I'd add the correct file extension to all existing manifests, though. Then I would add the hard validation for the bin fields of windows plugins, otherwise we would need to keep that in mind when reviewing manifests in the future. Seems cleaner and won't be too much work with the current number of plugins.

@ahmetb
Copy link
Member Author

ahmetb commented Aug 26, 2019

Actually I was surprised one of the PRs in krew-index mentioned that Windows’s WSL support can actually support non .exe files (e.g. bash scripts). kubernetes-sigs/krew-index#208 (comment)

I'm not wondering what sort of trickery is this. I expect WSL to return GOOS=linux as it's a linux emulation of some sort. In that case, windows platform spec should not be matching.

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 24, 2019
@kubernetes-sigs kubernetes-sigs deleted a comment from fejta-bot Nov 24, 2019
@kubernetes-sigs kubernetes-sigs deleted a comment from fejta-bot Nov 24, 2019
@kubernetes-sigs kubernetes-sigs deleted a comment from fejta-bot Nov 24, 2019
@ahmetb
Copy link
Member Author

ahmetb commented Nov 24, 2019

I've tried this on windows.

Here are some findings. On Windows:

  1. A kubectl plugin symlink must have an executable file extension. This is because golang uses exec.LookPath which uses PATHEXT environment variable (which is a ;-separated list, like ".com;.exe;.bat;.cmd").

  2. The file pointed by the symlink does not have be an executable (.exe) file, it can have no extension, or be even .txt/.png. But when it's executed as a kubectl plugin (through it's symlink), it will be treated as an executable.

Not sure where/if we can document this.

The only thing to know out of this is, the value of bin: field on windows does not have to be .exe. But we'll always link it as .exe, so for example if it was .py, it will be tried to exec as win32 app –and fail.

/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Nov 24, 2019
@ahmetb
Copy link
Member Author

ahmetb commented Feb 18, 2020

I think we should document somewhere that only .exe extensions are allowed for windows and don't support the rest.

@smaftoul
Copy link

Or should we support either bat or ps1 files (or both) and instruct people shipping windows plugins with the method we choose ?

@ahmetb
Copy link
Member Author

ahmetb commented Jan 28, 2022

@smaftoul Support request for ps1/bat files has not come up at all despite 115 of 181 plugins (63%) distribute on Windows. I'm inclined to think this is not necessary and there's not enough proof that this is required.

Most plugins are implemented in Go and bash, and for Windows support, people already opt in to use Go, and I'm yet to see a plugin developed in PS, so I think this is a wontfix for the time being. The task here is probably just to document that only ".exe" entrypoints work on windows somewhere on https://krew.sigs.k8s.io/docs/developer-guide/plugin-manifest/.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants