Add icon & file info to syncthing.exe

[harrisonhjones]

Purpose

This change fixes/addresses #3668 by adding an Icon to the Windows syncthing.exe binary.

Testing

• Ran go run build.go - success
• Verified thebinary was created atbin/syncthing.exe` - success
• Verified the binary had the Syncthing icon - success
• Verified the binary had the given file properties by right clicking on it, clicking the "properties" menu item, and then the "details" tab. - success

Note, I tested this build on a Windows machine.

Screenshots

Documentation

I don't believe this requires a documentation change.

Authorship

Every author of a code contribution (Go, Javascript, HTML, CSS etc, with the
possible exception of minor typo corrections and similar) is recorded in the
AUTHORS and NICKS files and the in-GUI credits. If this is your first
contribution, a maintainer will add you properly before accepting the
contribution. You need not do so yourself or worry about the fact that the
"authors" automated test fails. However, if your name (such as you want it
presented in the credits) is not visible on your Github profile or in your
commit messages, please assist by providing it here.

• Name: Harrison Jones (or Harrison H. Jones if middle initials are allowed)
• Email: harrisonhjones@gmail.com and/or harrison@harrisonhjones.com

[AudriusButkevicius]

I think vendoring is not done the right way. You should use gvt tool which should then update the manifest.

I am not sure this syso file should live in the cmd directory, perhaps the build directory or some temp directory?

Also, I assume you only want to do this for GOOS=windows only, yet potentially support cross compiling.

[AudriusButkevicius]

We should also potentially populate File Version.

[harrisonhjones]

I think vendoring is not done the right way. You should use gvt tool which should then update the manifest.

Got it.

I am not sure this syso file should live in the cmd directory, perhaps the build directory?

I believe it has to be put in the cmd directory so the go build tool will automatically pick it up when building the Windows binary.

Also, I assume you only want to do this for GOOS=windows.

Doesn't really matter since it doesn't impact other builds but this is only needed when building for Windows. I could add a platform check to buildSyso and then update the cleanup step so it doesn't fail if the syso isn't created.

We should also potentially populate File Version.

I wasn't sure what to update it to. Unfortunately, contrary to what the goversioninfo library would have you believe you can't supply any arbitrary string, you have to supply x.y.z.a numbers. I didn't see these readily available. We could parse them from the version perhaps? And fallback to something like 0.0.0.0 if the parse fails?

[AudriusButkevicius]

Right, I misunderstood how the syso stuff works. It seem the compiler takes care of it.

Well our version as it stands now is 0.14.something, so that fits in nicely with 0.14.something.somethingelse.

Where something else could be 0 in most cases, or N if we are ahead of some specific version tag by N commits (N is that +5 in the log version tag). Or even start of the commit hash converted from hex to dec.

[harrisonhjones]

For the file version, as long as we stick with the XX.YY.ZZ versioning we have the first 3 number of the version. For the 4th number we could use the release candidate number but that would tightly couple us to the tag format/layout. I'll look into using the commit hash converted into a decimal number.

[harrisonhjones]

Here's what file properties looks like with the new code:

From the failing checks I'm going to need some help with the vendoring. I ran the command gvt fetch github.com/josephspurrier/goversioninfo from the root of the syncthing directory, I can see the package is in the vendor directory and the manifest appears to be properly updated but the builds are still failing due to not being able to find goversioninfo. Thoughts?

[imsodin]

Even if not strictly necessary, I would still wrap it in if goos == "windows" - it's clear and more concise than the comment. Using defer you can put shouldCleanupSyso into the same if block.

[harrisonhjones]

Good idea! Hopefully I can blame completely forgetting defer exists on being sick :|

[imsodin]

What is the purpose of the file version as opposed to the product version? Generally I think it is a bad idea to put the number of commits since release as the last version number - that will create more confusion than be of help to anyone. I would just set it to 0 (if we need file version anyway).

The vendoring seems perfectly fine and works locally for me, I don't know what the build servers problem is (it's the first external dependency in build.go, but I don't see how that's different).

[harrisonhjones]

If we had some way to track different versions for each binary file version would be more useful but since all the binaries are generated at the same time there product version will ~= file version but since it's simple to populate I don't see why we shouldn't.

As for the final "build" version, I like the idea of making it equal to the number of commits ahead as it (should) make it easier to determine the exact commit the binary was built from. Why do you think it would cause confusion?

How do you test the vendoring locally?

[imsodin]

I just build it (without having it in my normal GOPATH, only vendor dir) and it works.

I can't follow how the time when a binary was built has anything to do with its version. For me a version is a tagged commit - it doesn't matter when the binary is built (I know about the reproducible build issue, but that's hardly what you are after here).

I don't think commits ahead are relevant for developers, at least I never came into the situation where I came upon a syncthing binary and needed to know which commit it is from - I just remove it and build whatever commit I need. For the user it is just as useless (what is a commit?) and might prompt confusion about a version that doesn't seem to exist in our releases at all.

[harrisonhjones]

I can't follow how the time when a binary was built has anything to do with its version. For me a version is a tagged commit - it doesn't matter when the binary is built (I know about the reproducible build issue, but that's hardly what you are after here).

Sorry, I wasn't super clear. "Time" doesn't really matter here. What I was trying to suggest was that if we set the "product version" to the release version "0.14.43" and we somehow built the binaries from different commits the file versions could be different than the product version. We don't do that however.

I don't think commits ahead are relevant for developers, at least I never came into the situation where I came upon a syncthing binary and needed to know which commit it is from - I just remove it and build whatever commit I need. For the user it is just as useless (what is a commit?) and might prompt confusion about a version that doesn't seem to exist in our releases at all.

It might be useful in bug reports. If a user files a bug we could ask for the full version and then we'd know exactly what commit their version was compiled from.

[imsodin]

It might be useful in bug reports. If a user files a bug we could ask for the full version and then we'd know exactly what commit their version was compiled from.

That information in the product version as per your screenshot and it's also obtainable with syncthing -version. That's why I personally don't see any benefit. My argument that it can cause confusion is also kind of moot, as users will only ever get released versions (i.e. the last number is 0). So in the end it is a personal feeling that it seems wrong that something that looks like a version (a.b.c.d) actually is not a version (as in if you search for it, you won't find anything).

Also we are bikeshedding hard here, the actual issue is the weird build problem - hopefully it's obvious and @AudriusButkevicius or @calmh will see it (tomorrow).

[AudriusButkevicius]

Let's put it this way, what's the downside of adding commits ahead? This just adds yet another way to find non-clean builds.

[calmh]

So yeah the build issue is nontrivial. Or, I mean, it's trivial to the extent that a go get github.com/josephspurrier/goversioninfo on the build server would pull in the package into GOPATH and things would work. But we don't want to require everyone to do that on all platforms, so build.go should really only import the standard library packages and nothing else.

However, this could be moved into a separate script in script/ like the stuff we use for generating translation files and so on, which is only run on Windows, and perhaps does not fail the build if it does not manage to create the syso.

The only way to get it to use vendored dependencies even in that case would be to build it to a binary and then run it. But what we typically do instead is mention the dependency in the setup() phase in build.go, and it'll be run at some point on the build server at least.

But if we're going down that path we might as well go install the goversioninfo binary and use that directly to generate the thing. To do that, just add it to the setup() part and use it in the regular build, on Windows. Again, it not being present should ideally not fail the build.

I'm not entirely up to date on Windows versioning semantics, but the "product version" being the full string as in the screenshot looks reasonable. If another field is limited to four digits, setting the last digits to the +commits part of the version string seems perfectly fine to me. (I think in Windows the fourth number is an ever increasing build number, which is bizarre when you're used to more minor number resetting when a more major component is bumped, but I don't think this matters.)

[harrisonhjones]

@calmh I'm new to vendoring but why don't the build machines use the vendored version of github.com/josephspurrier/goversioninfo when building?

[calmh]

They do. But you're not building anything. build.go is run as go run build.go.

I'd also be sort of happy if that didn't matter, but that's how the go compiler works.

[imsodin]

And the "reason" why go run doesn't care about vendor/ while go build does:
golang/go#20406 (comment)
I didn't know about this, that's why I said it worked for me earlier.

[AudriusButkevicius]

This is still very cool and we should still do this.

[calmh]

So practically I suggest,

• Add the goversioninfo package/binary here:

  syncthing/build.go

  Line 366 in 80079e8

  func setup() {

• Assume it is present in the build environment and just run the binary
• Make failure non-fatal

I'll make sure it gets installed in our build environment.

[harrisonhjones]

Thanks for the guidance. I'll attempt to address this this weekend.

[AudriusButkevicius]

I'd still love this to be there 😢

[harrisonhjones]

Finally found time for this. new PR: #4839