Portable plugin executable check for Windows platform #367

navidshaikh · 2019-08-15T12:49:22Z

Fixes #365

Proposed Changes

The code used for checking whether plugin is executable or not in plugin verifier
does not build for Windows. Builds Windows and non-Windows platform plugin verifier separately

knative-prow-robot · 2019-08-15T12:49:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: navidshaikh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [navidshaikh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

knative-metrics-robot · 2019-08-15T12:50:57Z

The following is the coverage report on pkg/.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/plugin/plugin_verifier.go	78.2%	74.2%	-3.9

navidshaikh · 2019-08-15T12:52:22Z

kn builds with this change for Windows

➜  client git:(fix-365)  GO111MODULE=on GOOS=windows GOARCH=amd64 go build -mod=vendor ./cmd/... && ll kn.exe
-rwxr-xr-x. 1 nshaikh nshaikh 36M Aug 15 18:21 kn.exe

duglin · 2019-08-15T13:17:05Z

I'm not sure we should remove the check since having a non-executable text file in your PATH is valid, but shouldn't show up as a plug-in when you plugin list.

If the real issue is that we can't compile certain files in Windows, can we just use golang's "build tags" and have the Windows version of that func be a no-op ?

navidshaikh · 2019-08-15T13:48:42Z

build tags are added per file basis AFAIK, we have executable check done as method, let mecheck if I can do better to fix this

navidshaikh · 2019-08-15T14:18:19Z

I tried and separated the non Windows platform check in separate file and add +build !windows build flag to it, however that non-windows-platform check has to be referenced in original plugin verifier and not-building it for windows makes it undefined

pkg/kn/commands/plugin/plugin_verifier.go:131:9: undefined: checkForNonWindowsExecutable

may be we'll need to delegate the executable check to underlying platform

duglin · 2019-08-15T14:39:05Z

can't you put the funcs that have linux vs windows versions into their own files and then only compile with the OS specific version?

navidshaikh · 2019-08-15T14:43:41Z

Wondering how come the nightly build has it built https://github.com/knative/client/blob/master/docs/README.md#installing-kn
or is it pointing to last built kn before plugin verifier PR merged in

navidshaikh · 2019-08-15T15:34:32Z

can't you put the funcs that have linux vs windows versions into their own files and then only compile with the OS specific version?

You are right! Didn't do better yet to fix this. :-/

builds Windows and non-Windows platform plugin verifier separately

pkg/kn/commands/plugin/plugin_verifier_windows.go

navidshaikh · 2019-08-15T15:43:43Z

pkg/kn/commands/plugin/plugin_verifier.go

@@ -12,6 +12,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

+// +build !windows


builds for non-Windows platform

So, when we mention GOOS=windows while building cross platform binary it skips this file and picks up Windows specific version defining the needed methods / functions to verify plugin on Windows platform.

navidshaikh · 2019-08-15T15:44:18Z

pkg/kn/commands/plugin/plugin_verifier_windows.go

@@ -0,0 +1,190 @@
+// Copyright © 2019 The Knative Authors


Removed all the functions which are in plugin_verifier.go which are specific to non-Windows platform

Seems like a lot of that code could be in a common file and limit the plugin_verifier_windows.go with only the Windows code. Is there a reason you could not do this?

navidshaikh · 2019-08-15T15:50:19Z

Builds for all platform now

21:15 ➜  client git:(fix-365) ✗  ./hack/build-cross.sh 
🚧 🐧 Building for Linux
🚧 🍏 Building for macOS
🚧 🎠 Building for Windows
21:15 ➜  client git:(fix-365) ✗  ll kn-*
-rwxr-xr-x. 1 nshaikh nshaikh 36M Aug 15 21:15 kn-darwin-amd64
-rwxr-xr-x. 1 nshaikh nshaikh 36M Aug 15 21:15 kn-linux-amd64
-rwxr-xr-x. 1 nshaikh nshaikh 36M Aug 15 21:15 kn-windows-amd64.exe

Used the code from hack/release.sh in hack/build-cross.sh as below

21:16 ➜  client git:(fix-365) ✗  tail -n 17 hack/build-cross.sh
source $(dirname $0)/../vendor/knative.dev/test-infra/scripts/release.sh
source $(dirname $0)/build-flags.sh

ld_flags="$(build_flags $(dirname $0)/..)"
pkg="github.com/knative/client/pkg/kn/commands"
version="${TAG}"
# Use vYYYYMMDD-<hash>-local for the version string, if not passed.
[[ -z "${version}" ]] && version="v${BUILD_TAG}-local"

export GO111MODULE=on
export CGO_ENABLED=0
echo "🚧 🐧 Building for Linux"
GOOS=linux GOARCH=amd64 go build -mod=vendor -ldflags "${ld_flags}" -o ./kn-linux-amd64 ./cmd/...
echo "🚧 🍏 Building for macOS"
GOOS=darwin GOARCH=amd64 go build -mod=vendor -ldflags "${ld_flags}" -o ./kn-darwin-amd64 ./cmd/...
echo "🚧 🎠 Building for Windows"
GOOS=windows GOARCH=amd64 go build -mod=vendor -ldflags "${ld_flags}" -o ./kn-windows-amd64.exe ./cmd/...

navidshaikh · 2019-08-15T16:09:47Z

/test pull-knative-client-build-tests

As we'll need either to mention, which file compiles for which platform.

navidshaikh · 2019-08-15T18:20:51Z

Summary:

We don't compile syscall.Stat_t for Windows platform where it isn't available, but for all non-Windows platform
Introduced plugin_verifier_windows.go which will be compiled only for Windows platform and doesn't include syscall.Stat_t in that file and respective methods / function.
(We used suffix _windows.go to flag that)
Based on the platform, respective file (plugin_verifier.go or plugin_verifier_windows.go) land into the binary.

This should unblock the cross-platform builds.

navidshaikh · 2019-08-15T18:24:56Z

@maximilien @sixolet @duglin : I've verified this PR by building it for cross platform as mentioned in above comments, I think this is ready for review.
(would be good if have it in and unblock our nightly build, current nightly build is 7 days old)

//Cc: @adrcunha

maximilien

/ok_to_test

maximilien · 2019-08-15T20:29:09Z

pkg/kn/commands/plugin/plugin_verifier_windows.go

@@ -0,0 +1,190 @@
+// Copyright © 2019 The Knative Authors


Seems like a lot of that code could be in a common file and limit the plugin_verifier_windows.go with only the Windows code. Is there a reason you could not do this?

navidshaikh · 2019-08-15T20:58:32Z

Seems like a lot of that code could be in a common file and limit the plugin_verifier_windows.go with only the Windows code. Is there a reason you could not do this?

@maximilien : It's the other way around actually, there are specific syscalls which are absent for
Windows platforms, in this case syscall.Stat_t.

And we can't reference the specific-platform-functions from a common function,
because those specific-platform-functions will be visited compile time, in all the platforms build (as we'll not mark build tags for file having common function).

We'll need to completely separate references and imports at compile time for Windows and non-Windows platforms, to mask the unavailable syscalls imports.

Marking plugin_verifier.go file, which uses of syscall.Stat_t as +build !windows ,
hides it compile time when GOOS=windows
Having filename ending with _windows.go hides it compile time,
when GOOS=linux or GOOS=darwin to avoid re-declaration of vars / functions.

sixolet · 2019-08-15T21:59:47Z

/lgtm

maximilien · 2019-08-16T00:37:19Z

@maximilien : It's the other way around actually, there are specific syscalls which are absent for
Windows platforms, in this case syscall.Stat_t.

OK cool. Thanks. I have no way to test nor use Windows, so glad you are on it. Thx

…ve#367)

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 15, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 15, 2019

knative-prow-robot requested review from mattmoor and sixolet August 15, 2019 12:49

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 15, 2019

mattmoor removed their request for review August 15, 2019 14:19

navidshaikh force-pushed the fix-365 branch from ebf9e87 to 8361e3f Compare August 15, 2019 15:37

navidshaikh changed the title ~~Removes check whether plugin is executable~~ Portable plugin executable check for Windows platform Aug 15, 2019

Portable plugin executable check for Windows platform

494c323

builds Windows and non-Windows platform plugin verifier separately

navidshaikh force-pushed the fix-365 branch from 8361e3f to 494c323 Compare August 15, 2019 15:41

navidshaikh commented Aug 15, 2019

View reviewed changes

pkg/kn/commands/plugin/plugin_verifier_windows.go Outdated Show resolved Hide resolved

navidshaikh commented Aug 15, 2019

View reviewed changes

Removes redundant build tag and file suffix, uses file suffix

d6a4944

As we'll need either to mention, which file compiles for which platform.

maximilien reviewed Aug 15, 2019

View reviewed changes

knative-prow-robot assigned sixolet Aug 15, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 15, 2019

knative-prow-robot merged commit 54d024f into knative:master Aug 15, 2019

navidshaikh deleted the fix-365 branch August 16, 2019 06:07

dsimansk added a commit to dsimansk/client that referenced this pull request Mar 8, 2024

[release-v1.11] Update cloudevents/sdk-go/v2 to latest release (knati…

37b85b9

…ve#367)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Portable plugin executable check for Windows platform #367

Portable plugin executable check for Windows platform #367

navidshaikh commented Aug 15, 2019 •

edited

Loading

knative-prow-robot commented Aug 15, 2019

knative-metrics-robot commented Aug 15, 2019

navidshaikh commented Aug 15, 2019

duglin commented Aug 15, 2019

navidshaikh commented Aug 15, 2019

navidshaikh commented Aug 15, 2019

duglin commented Aug 15, 2019

navidshaikh commented Aug 15, 2019

navidshaikh commented Aug 15, 2019

navidshaikh Aug 15, 2019

navidshaikh Aug 15, 2019

navidshaikh Aug 15, 2019

maximilien Aug 15, 2019

navidshaikh commented Aug 15, 2019

navidshaikh commented Aug 15, 2019

navidshaikh commented Aug 15, 2019 •

edited

Loading

navidshaikh commented Aug 15, 2019 •

edited

Loading

maximilien left a comment

maximilien Aug 15, 2019

navidshaikh commented Aug 15, 2019

sixolet commented Aug 15, 2019

maximilien commented Aug 16, 2019

Portable plugin executable check for Windows platform #367

Portable plugin executable check for Windows platform #367

Conversation

navidshaikh commented Aug 15, 2019 • edited Loading

Proposed Changes

knative-prow-robot commented Aug 15, 2019

knative-metrics-robot commented Aug 15, 2019

navidshaikh commented Aug 15, 2019

duglin commented Aug 15, 2019

navidshaikh commented Aug 15, 2019

navidshaikh commented Aug 15, 2019

duglin commented Aug 15, 2019

navidshaikh commented Aug 15, 2019

navidshaikh commented Aug 15, 2019

navidshaikh Aug 15, 2019

Choose a reason for hiding this comment

navidshaikh Aug 15, 2019

Choose a reason for hiding this comment

navidshaikh Aug 15, 2019

Choose a reason for hiding this comment

maximilien Aug 15, 2019

Choose a reason for hiding this comment

navidshaikh commented Aug 15, 2019

navidshaikh commented Aug 15, 2019

navidshaikh commented Aug 15, 2019 • edited Loading

navidshaikh commented Aug 15, 2019 • edited Loading

maximilien left a comment

Choose a reason for hiding this comment

maximilien Aug 15, 2019

Choose a reason for hiding this comment

navidshaikh commented Aug 15, 2019

sixolet commented Aug 15, 2019

maximilien commented Aug 16, 2019

navidshaikh commented Aug 15, 2019 •

edited

Loading

navidshaikh commented Aug 15, 2019 •

edited

Loading

navidshaikh commented Aug 15, 2019 •

edited

Loading