code review 160950044: misc/makerelease: set version number in Windows installer

misc/makerelease: set version number in Windows installer

Set correct version number at Windows installer based on
Go's Mercurial tag.

Name                                     | Version
------------------------------------------------
Go Programming Language amd64 go1.3.3    | 1.3.3
Go Programming Language amd64 go1.2rc3   | 1.2
Go Programming Language amd64 go1.2beta1 | 1.2

Fixes issue 8239.

[jfrederich, 2014-10-16 16:01:49]

Hello rsc@golang.org, adg@golang.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[cehessman, 2014-10-17 04:00:48]

https://codereview.appspot.com/160950044/diff/60001/misc/makerelease/makerele...
File misc/makerelease/makerelease.go (right):

https://codereview.appspot.com/160950044/diff/60001/misc/makerelease/makerele...
misc/makerelease/makerelease.go:474: // The Microsoft installer requires version
format MAJOR.MINOR.BUILD.UPDATE.
Source? According to
http://msdn.microsoft.com/en-us/library/aa370859(v=vs.85).aspx, the format of
the string is major.minor.build

https://codereview.appspot.com/160950044/diff/60001/misc/makerelease/makerele...
misc/makerelease/makerelease.go:476: // The offical Golang version format is
goMAJOR.MINOR.PATCH at $GOROOT/VERSION.
s/offical/official
s/Golang/Go

[brainman, 2014-10-17 05:37:31]

It looks simple enough, thank you. I will try it early next week, and report
back here.

Alex

[adg, 2014-10-17 07:46:36]

https://codereview.appspot.com/160950044/diff/60001/misc/makerelease/makerele...
File misc/makerelease/makerelease.go (right):

https://codereview.appspot.com/160950044/diff/60001/misc/makerelease/makerele...
misc/makerelease/makerelease.go:479: // Should we handle release candidates like
go1.3rc2? If so what is
Don't worry about release candidates.

This logic should be

var versionRe = regexp.MustCompile(`^go([0-9]+(\.[0-9]+)*)`)

func wixVersion(v string) string {
  m := verisonRe.FindStringSubmatch(v)
  if m == nil {
    return v
  }
  return m[1]
}

[jfrederich, 2014-10-17 08:52:12]

On 2014/10/17 04:00:48, ceh wrote:
>
https://codereview.appspot.com/160950044/diff/60001/misc/makerelease/makerele...
> File misc/makerelease/makerelease.go (right):
> 
>
https://codereview.appspot.com/160950044/diff/60001/misc/makerelease/makerele...
> misc/makerelease/makerelease.go:474: // The Microsoft installer requires
version
> format MAJOR.MINOR.BUILD.UPDATE.
> Source? According to
> http://msdn.microsoft.com/en-us/library/aa370859(v=vs.85).aspx, the format of
> the string is major.minor.build
> 
I think the Microsoft manual is not up-to-date. The note "Note that Windows
Installer uses only the first three fields of the product version. If you
include a fourth field in your product version, the installer ignores the fourth
field." This is not true on my Windows 7 Pro x64 system. The installer doesn't
ignore the fourth field.

Jens

[jfrederich, 2014-10-17 10:26:54]

On 2014/10/17 07:46:36, adg wrote:
>
https://codereview.appspot.com/160950044/diff/60001/misc/makerelease/makerele...
> File misc/makerelease/makerelease.go (right):
> 
>
https://codereview.appspot.com/160950044/diff/60001/misc/makerelease/makerele...
> misc/makerelease/makerelease.go:479: // Should we handle release candidates
like
> go1.3rc2? If so what is
> Don't worry about release candidates.
> 
> This logic should be
> 
> var versionRe = regexp.MustCompile(`^go([0-9]+(\.[0-9]+)*)`)
> 
> func wixVersion(v string) string {
>   m := verisonRe.FindStringSubmatch(v)
>   if m == nil {
>     return v
>   }
>   return m[1]
> }

Andrew, I agree with you. That's the elegant solution. But if you want to encode
the 'rc' or 'beta' value at the fourth field then is fails. That's the reason
for my incomplete first CL.

Now the output is:

Name                                     | Version
--------------------------------------------------
Go Programming Language amd64 1.3.3      | 1.3.3
Go Programming Language amd64 go1.2rc3   | 1.2       
Go Programming Language amd64 go1.2beta1 | 1.2

Jens

[jfrederich, 2014-10-17 10:27:14]

Hello adg@golang.org, c.emil.hessman@gmail.com, alex.brainman@gmail.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[adg1, 2014-10-19 22:37:19]

https://codereview.appspot.com/160950044/diff/90001/misc/makerelease/makerele...
File misc/makerelease/makerelease.go (right):

https://codereview.appspot.com/160950044/diff/90001/misc/makerelease/makerele...
misc/makerelease/makerelease.go:482: re :=
regexp.MustCompile(`^go([0-9]+(\.[0-9]+)*)`)
this should be a global variable

https://codereview.appspot.com/160950044/diff/90001/misc/makerelease/makerele...
misc/makerelease/makerelease.go:485: return v // installer fails: 'goX.X.X' is
invalid
in this case, return "0.0.0" or something that will not break the installer

[brainman, 2014-10-20 01:27:26]

I can confirm that it does fixes issue 8239. Thank you.

Leaving for adg.

Alex

https://codereview.appspot.com/160950044/diff/90001/misc/makerelease/windows/...
File misc/makerelease/windows/installer.wxs (left):

https://codereview.appspot.com/160950044/diff/90001/misc/makerelease/windows/...
misc/makerelease/windows/installer.wxs:21: Name="Go Programming Language
$(var.Arch) $(var.Version)"
I could not apply your patch.

# hg clpatch 160950044
edit misc/makerelease/windows/installer.wxs: patch did not apply cleanly
abort: hgapplydiff failed

As far as I see, you assume that installer.wxs uses \n for line ends. It does
not, it uses \r\n. I am not sure if it matters, so maybe leave it as is.

[jfrederich, 2014-10-20 07:57:56]

Please take another look.

https://codereview.appspot.com/160950044/diff/90001/misc/makerelease/windows/...
File misc/makerelease/windows/installer.wxs (left):

https://codereview.appspot.com/160950044/diff/90001/misc/makerelease/windows/...
misc/makerelease/windows/installer.wxs:21: Name="Go Programming Language
$(var.Arch) $(var.Version)"
On 2014/10/20 01:27:26, brainman wrote:
> I could not apply your patch.
> 
> # hg clpatch 160950044
> edit misc/makerelease/windows/installer.wxs: patch did not apply cleanly
> abort: hgapplydiff failed
> 
> As far as I see, you assume that installer.wxs uses \n for line ends. It does
> not, it uses \r\n. I am not sure if it matters, so maybe leave it as is.

I don't know what went wrong. On my machine the installer.wxs is in dos file
format and my editor create a dos diff for installer.wxs. But in combination
with the makerelease.go diff (which is in unix file format) the result CL is in
unix file format. 

So why don't we change the installer.wxs to unix file format too? I've tested
it, it works well.

Jens

[adg1, 2014-10-20 20:44:12]

LGTM

[brainman, 2014-10-20 22:37:53]

On Tue, Oct 21, 2014 at 7:48 AM, Andrew Gerrand <adg@golang.org> wrote:
> The patch doesn't apply cleanly to installer.wxs.
>
> Can you "hg sync" and then run "hg upload 160950044" ?
>

Like I said in #9, "hg sync" will not help. The patch assumes that installer.wxs
has lines delimited by \n:

# wget -qO- https://codereview.appspot.com/download/issue160950044_130001.diff |
sed '/installer.wxs/,$!d' | cat -v
Index: misc/makerelease/windows/installer.wxs
===================================================================
--- a/misc/makerelease/windows/installer.wxs
+++ b/misc/makerelease/windows/installer.wxs
@@ -18,13 +18,12 @@
 
 <Product
     Id="FF5B30B2-08C2-11E1-85A2-6ACA4824019B"
-    Name="Go Programming Language $(var.Arch) $(var.Version)"
+    Name="Go Programming Language $(var.Arch) $(var.GoVersion)"
     Language="1033"
     Codepage="1252"
-    Version="0.0.0.0"
+    Version="$(var.WixGoVersion)"
     Manufacturer="http://golang.org"
     UpgradeCode="$(var.UpgradeCode)" >
-    <!-- Version="$(var.Version)" TODO: Version requires X.X.X.X format -->
 
 <Package
     Id='*' 

while the file has \r\n at line ends:

# cat -v $GOROOT/misc/makerelease/windows/installer.wxs | grep var.Version
    Name="Go Programming Language $(var.Arch) $(var.Version)"^M
    <!-- Version="$(var.Version)" TODO: Version requires X.X.X.X format -->^M

Alex

[adg1, 2014-10-21 01:39:37]

So this is an issue with our code review tool?

On 21 October 2014 09:37, <alex.brainman@gmail.com> wrote:

> On Tue, Oct 21, 2014 at 7:48 AM, Andrew Gerrand <adg@golang.org> wrote:
>
>> The patch doesn't apply cleanly to installer.wxs.
>>
>
>  Can you "hg sync" and then run "hg upload 160950044" ?
>>
>
>
> Like I said in #9, "hg sync" will not help. The patch assumes that
> installer.wxs has lines delimited by \n:
>
> # wget -qO-
> https://codereview.appspot.com/download/issue160950044_130001.diff | sed
> '/installer.wxs/,$!d' | cat -v
> Index: misc/makerelease/windows/installer.wxs
> ===================================================================
> --- a/misc/makerelease/windows/installer.wxs
> +++ b/misc/makerelease/windows/installer.wxs
> @@ -18,13 +18,12 @@
>
>  <Product
>      Id="FF5B30B2-08C2-11E1-85A2-6ACA4824019B"
> -    Name="Go Programming Language $(var.Arch) $(var.Version)"
> +    Name="Go Programming Language $(var.Arch) $(var.GoVersion)"
>      Language="1033"
>      Codepage="1252"
> -    Version="0.0.0.0"
> +    Version="$(var.WixGoVersion)"
>      Manufacturer="http://golang.org"
>      UpgradeCode="$(var.UpgradeCode)" >
> -    <!-- Version="$(var.Version)" TODO: Version requires X.X.X.X format
> -->
>
>  <Package
>      Id='*'
>
> while the file has \r\n at line ends:
>
> # cat -v $GOROOT/misc/makerelease/windows/installer.wxs | grep
> var.Version
>     Name="Go Programming Language $(var.Arch) $(var.Version)"^M
>     <!-- Version="$(var.Version)" TODO: Version requires X.X.X.X format
> -->^M
>
> Alex
>
> https://codereview.appspot.com/160950044/
>

[brainman, 2014-10-21 01:51:07]

On 2014/10/21 01:39:37, adg1 wrote:
> So this is an issue with our code review tool?
> 

Our code review tool does not generates patches manually, we use whatever hg
provides. I suspect there are some "line end translation" settings in hg that
might be at fault here. But I don't know for sure, I don't use code review on
windows. Lets wait for jfrederich, he would know what he did.

Perhaps it is good idea to change all line ends in all those file, if it makes
no difference to WiX tools. But I think we should do it in a separate CL.

Alex

[adg1, 2014-10-21 01:56:58]

Agree. If Jens can't figure it out I'll just make the changes locally by
hand and submit.

On 21 October 2014 12:51, <alex.brainman@gmail.com> wrote:

> On 2014/10/21 01:39:37, adg1 wrote:
>
>> So this is an issue with our code review tool?
>>
>
>
> Our code review tool does not generates patches manually, we use
> whatever hg provides. I suspect there are some "line end translation"
> settings in hg that might be at fault here. But I don't know for sure, I
> don't use code review on windows. Lets wait for jfrederich, he would
> know what he did.
>
> Perhaps it is good idea to change all line ends in all those file, if it
> makes no difference to WiX tools. But I think we should do it in a
> separate CL.
>
> Alex
>
> https://codereview.appspot.com/160950044/
>

[jfrederich, 2014-10-21 16:47:20]

Andrew, please make the changes by hand this time.

I think yes it's an issue with the code review tool. Translating the line ending
of installer.wxs to Unix is not the right fix, it fixes the symptoms. I've three
development setups in use (lin, osx, and win). It's easily reproducible on all
of them. I've filed a new issue
(https://code.google.com/p/go/issues/detail?id=8975) for it. I'll find out what
goes wrong. Andrew, or Alex can anyone set the right people of the issue CC list
please? I've no access to do that.

Jens

On 2014/10/21 01:56:58, adg1 wrote:
> Agree. If Jens can't figure it out I'll just make the changes locally by
> hand and submit.
> 
> On 21 October 2014 12:51, <mailto:alex.brainman@gmail.com> wrote:
> 
> > On 2014/10/21 01:39:37, adg1 wrote:
> >
> >> So this is an issue with our code review tool?
> >>
> >
> >
> > Our code review tool does not generates patches manually, we use
> > whatever hg provides. I suspect there are some "line end translation"
> > settings in hg that might be at fault here. But I don't know for sure, I
> > don't use code review on windows. Lets wait for jfrederich, he would
> > know what he did.
> >
> > Perhaps it is good idea to change all line ends in all those file, if it
> > makes no difference to WiX tools. But I think we should do it in a
> > separate CL.
> >
> > Alex
> >
> > https://codereview.appspot.com/160950044/
> >

[adg, 2014-10-21 23:56:39]

*** Submitted as https://code.google.com/p/go/source/detail?r=4b5ffe0e679e ***

misc/makerelease: set version number in Windows installer

Set correct version number at Windows installer based on
Go's Mercurial tag.

Name                                     | Version
------------------------------------------------
Go Programming Language amd64 go1.3.3    | 1.3.3
Go Programming Language amd64 go1.2rc3   | 1.2
Go Programming Language amd64 go1.2beta1 | 1.2

Fixes issue 8239.

LGTM=adg
R=adg, c.emil.hessman, alex.brainman
CC=golang-codereviews
https://codereview.appspot.com/160950044

Committer: Andrew Gerrand <adg@golang.org>

[gobot, 2014-10-22 00:28:04]

This CL appears to have broken the netbsd-amd64-bsiegert builder.
See http://build.golang.org/log/b9ccff760aab288c7ae2a44c167e96d158412592

[dave_cheney.net, 2014-10-22 00:37:34]

Can we just turn this builder off, it constantly try cries wolf. 



> On 22 Oct 2014, at 11:28, gobot@golang.org wrote:
> 
> This CL appears to have broken the netbsd-amd64-bsiegert builder.
> See http://build.golang.org/log/b9ccff760aab288c7ae2a44c167e96d158412592
> 
> https://codereview.appspot.com/160950044/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[adg1, 2014-10-22 00:45:17]

On 22 October 2014 11:37, Dave Cheney <dave@cheney.net> wrote:

> Can we just turn this builder off, it constantly try cries wolf.
>

https://codereview.appspot.com/155620047