Add windows detail info at compile time for binary oc.exe #14862

shiywang · 2017-06-23T14:39:11Z

Ref: https://trello.com/c/Cn8IaH8g/209-properly-sign-and-package-oc-binary-for-windows
I really know a little about bash programing, so I think it's better to have an early review to puts those things into the right places, @stevekuznetsov would you mind to take a look at this ? I feel that go get could be put to a better place, but I just don't know where.

@fabianofranz currently that goversioninfo will read information from windows_versioninfo.json file but that information can be overrided with some ENV I put here, I just not sure which field in windows_versioninfo.json shoud be static and which filed should be dynamic. currently the whole json file I just fill with some random number, not sure what exactly value shoud we fill.

also I tested that generated resource.syso file with a small hello world program running go install on windows, it works, I will make the whole build-cross.sh pass on windows as long as this pr has been polished enough.

sorry for the commit message was a little unclear, will squash and amend that when I back to the office

stevekuznetsov

mostly LGTM

stevekuznetsov · 2017-06-23T15:54:50Z

hack/lib/build/binaries.sh

@@ -219,6 +219,17 @@ os::build::internal::build_binaries() {
        local_ldflags+=" -s"
      fi

+      #Add Windows File Properties/Version Info and Icon Resource for oc.exe
+      if [[ $platform == "windows/amd64" ]]; then
+        os::build::version::get_vars


I don't think you should need this here.

I follow the code here, and when I remove this line, it throws src/github.com/openshift/origin/hack/lib/build/binaries.sh: line 225: OS_GIT_MAJOR: unbound variable

stevekuznetsov · 2017-06-23T16:00:44Z

hack/lib/build/binaries.sh

+      #Add Windows File Properties/Version Info and Icon Resource for oc.exe
+      if [[ $platform == "windows/amd64" ]]; then
+        os::build::version::get_vars
+        go get -u github.com/josephspurrier/goversioninfo


In the script here we should just

os::util::ensure::gopath_binary_exists 'goversioninfo'

Then we install the dep in the environment doing the build first, not as part of this process. We can add a feature to os::util::ensure::gopath_binary_exists that will go install if it is not present maybe

Then we install the dep in the environment doing the build first

do you mean following this doc https://github.com/kubernetes/community/blob/master/contributors/devel/godep.md to install dep?

Sorry, no. We can install the dependency we need on the CI machines by adding to this list. I just meant that we should keep the logic for installing dependencies out of our scripts here -- use os::util::ensure to print a nice message when something is missing, but heretofore we have not been in the business of installing stuff on people's machines when they run a script.

stevekuznetsov · 2017-06-23T16:01:47Z

hack/lib/build/binaries.sh

+        os::build::version::get_vars
+        go get -u github.com/josephspurrier/goversioninfo
+        local major="${OS_GIT_MAJOR}"
+        local minor=`echo ${OS_GIT_MINOR} |grep -o "[0-9]*"`


Is this just to strip out the +? Use:

local minor="${OS_GIT_MINOR%+}"

stevekuznetsov · 2017-06-23T16:02:32Z

hack/lib/build/binaries.sh

@@ -219,6 +219,17 @@ os::build::internal::build_binaries() {
        local_ldflags+=" -s"
      fi

+      #Add Windows File Properties/Version Info and Icon Resource for oc.exe
+      if [[ $platform == "windows/amd64" ]]; then


Quote & brace expansions:

if [[ "${platform}" == "windows/amd64" ]]; then ^ ^ ^^

stevekuznetsov · 2017-06-23T16:02:42Z

hack/lib/build/binaries.sh

@@ -234,6 +245,10 @@ os::build::internal::build_binaries() {
        fi
      fi

+      if [[ $platform == "windows/amd64" ]]; then


if [[ "${platform}" == "windows/amd64" ]]; then ^ ^ ^^

stevekuznetsov · 2017-06-23T16:03:10Z

hack/lib/build/binaries.sh

@@ -234,6 +245,10 @@ os::build::internal::build_binaries() {
        fi
      fi

+      if [[ $platform == "windows/amd64" ]]; then
+        rm ${OS_ROOT}/cmd/oc/resource.syso


Quote expansions

stevekuznetsov · 2017-06-23T16:05:47Z

pkg/version/windows_versioninfo.json

+		"FileVersion": {
+			"Major": 3,
+			"Minor": 6,
+			"Patch": 1234,


We can have one of these, we could update the version vars stuff to contain it.

stevekuznetsov · 2017-06-23T16:05:59Z

pkg/version/windows_versioninfo.json

+			"Major": 3,
+			"Minor": 6,
+			"Patch": 1234,
+			"Build": 1234


Not sure if we care, but this could be the Git SHA we build from

stevekuznetsov · 2017-06-23T16:06:50Z

pkg/version/windows_versioninfo.json

@@ -0,0 +1,44 @@
+{


The intent of this file is to act as a template that gets generated at build-time, right?

Right, this file has to be generated.

To generate it, @shiywang you should be able to get version information from env vars like OS_GIT_VERSION, OS_GIT_MAJOR (Major), OS_GIT_MINOR (Minor), OS_GIT_COMMIT (Patch). Date modified is just something like "$(date -u +'%Y-%m-%dT%H:%M:%SZ')" (formatted the way goversioninfo expects it).

Company Name, Product Name and Copyright can probably be static, unless we have it in a var somewhere (@stevekuznetsov clue?) or it differs for any of the products we build (Origin, OCP, Online). @brenton or @jupierce can you confirm this?

We have a function os::build::get_product_vars in hack/lib/build/binaries.sh that holds things we expect to change between Origin and OCP -- the name is not in there yet but we can definitely add it.

Sounds good.

the thing is some filed like you said (Patch) in our code OS_GIT_COMMIT is a string, but in versioninfo is int https://github.com/josephspurrier/goversioninfo/blob/master/goversioninfo.go#L50
I think we need to think how to map our current exist value to the right place, it will be much easier if we have those kind of mapping tables as a document.

What exactly are all the required fields?

Patch should be the Z in X.Y.Z -- right now we are not exposing it inside of the Bash library when we parse out the version but we can just add that.

shiywang · 2017-06-28T05:47:07Z

updated, and tested after cross build on windows, now it looks like this:

@fabianofranz @stevekuznetsov PTAL

stevekuznetsov · 2017-06-28T14:39:07Z

hack/lib/build/binaries.sh

+      if [[ "$platform" == "windows/amd64" ]]; then
+        os::build::version::get_vars
+        os::util::ensure::gopath_binary_exists 'goversioninfo'
+        local major="${OS_GIT_MAJOR}"


Are we not using these major and minor anymore? Seemed reasonable to use them, and you could update version.sh to also get you patch as well.

Re-added here: shiywang#2

fabianofranz · 2017-06-29T00:49:52Z

I think with shiywang#2 we're pretty much done, and if we could get this in for 3.6 to meet some customer's requests, it would be be great. @smarterclayton could you give this a last pass?

Handle major, minor, and patch in Windows versioninfo

shiywang · 2017-06-29T05:30:17Z

here is the new screenshot

stevekuznetsov · 2017-06-29T13:18:06Z

hack/lib/build/binaries.sh

@@ -219,6 +219,61 @@ os::build::internal::build_binaries() {
        local_ldflags+=" -s"
      fi

+      #Add Windows File Properties/Version Info and Icon Resource for oc.exe
+      if [[ "$platform" == "windows/amd64" ]]; then
+        os::build::version::get_vars


Only remaining comment is it feels weird to be calling this explicitly but I have not given the call-chain thought to see why we don't already have the version here.

I will take a look tomorrow if you guys don't have bandwidth digging this, also about that add + if patch exist @fabianofranz @stevekuznetsov wdyt ?

Yep, sounds good. If I have the bandwidth I'll also dig a little bit about this today. Tks @shiywang.

smarterclayton · 2017-06-29T15:10:51Z

hack/lib/build/version.sh

@@ -88,10 +89,11 @@ function os::build::version::kubernetes_vars() {
 	# Try to match the "git describe" output to a regex to try to extract
 	# the "major" and "minor" versions and whether this is the exact tagged
 	# version or whether the tree is between two tagged versions.
-	if [[ "${KUBE_GIT_VERSION}" =~ ^v([0-9]+)\.([0-9]+)(\.[0-9]+)*([-].*)?$ ]]; then
+	if [[ "${KUBE_GIT_VERSION}" =~ ^v([0-9]+)\.([0-9]+)\.([0-9]+)(\.[0-9]+)*([-].*)?$ ]]; then


Are kube tags now guaranteed to have 3 digits? That wasn't the case before.

Reverted in #14967 since we don't really need it for kube, just OS.

smarterclayton · 2017-06-29T15:11:03Z

hack/lib/build/version.sh

 				OS_GIT_MAJOR=${BASH_REMATCH[1]}
 				OS_GIT_MINOR=${BASH_REMATCH[2]}
-				if [[ -n "${BASH_REMATCH[4]}" ]]; then
+				OS_GIT_PATCH=${BASH_REMATCH[3]}


MICRO, not PATCH

Nm, I see that's the semantic versioning terminology.

smarterclayton · 2017-06-29T15:13:33Z

hack/lib/build/binaries.sh

+	}
+}
+EOF
+        goversioninfo -o ${OS_ROOT}/cmd/oc/oc.syso ${windows_versioninfo_file} 


Are you mutating a file inside of the normal source tree? That's generally something we don't do. Why is the file required to be in cmd/oc?

Yeah I was reluctant to do that for this same reason, but the compiler needs to have the .syso file in the same directory of main. We remove the file a few lines below.

smarterclayton · 2017-06-29T15:13:35Z

hack/lib/build/binaries.sh

@@ -219,6 +219,61 @@ os::build::internal::build_binaries() {
        local_ldflags+=" -s"
      fi

+      #Add Windows File Properties/Version Info and Icon Resource for oc.exe
+      if [[ "$platform" == "windows/amd64" ]]; then


Put this into a separate function, it makes this method hard to read.

Done in #14967.

fabianofranz · 2017-06-29T19:33:53Z

Closing this in favor of #14967 so that we can get it to the merge queue before @shiywang is back tomorrow. @smarterclayton @stevekuznetsov can you please give it a final pass there?

shiywang requested review from fabianofranz, stevekuznetsov and smarterclayton June 23, 2017 14:39

shiywang assigned fabianofranz Jun 23, 2017

stevekuznetsov suggested changes Jun 23, 2017

View reviewed changes

shiywang force-pushed the windows branch 2 times, most recently from 621ac26 to a453d84 Compare June 28, 2017 05:38

adding versioninfo data to Windows binaries

e5a6ffd

shiywang force-pushed the windows branch from a453d84 to e5a6ffd Compare June 28, 2017 05:40

stevekuznetsov reviewed Jun 28, 2017

View reviewed changes

Handle major, minor, and patch in Windows versioninfo

3f5d561

Merge pull request #2 from fabianofranz/windows_compile_time_info

cf20863

Handle major, minor, and patch in Windows versioninfo

stevekuznetsov reviewed Jun 29, 2017

View reviewed changes

smarterclayton reviewed Jun 29, 2017

View reviewed changes

fabianofranz mentioned this pull request Jun 29, 2017

Windows compile time info #14967

Merged

fabianofranz closed this Jun 29, 2017

Add windows detail info at compile time for binary oc.exe #14862

Add windows detail info at compile time for binary oc.exe #14862

Conversation

shiywang commented Jun 23, 2017 • edited

stevekuznetsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiywang Jun 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiywang commented Jun 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianofranz commented Jun 29, 2017

shiywang commented Jun 29, 2017

Choose a reason for hiding this comment

shiywang Jun 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianofranz commented Jun 29, 2017

shiywang commented Jun 23, 2017 •

edited

shiywang Jun 25, 2017 •

edited

shiywang Jun 29, 2017 •

edited