Embed the version information into the Windows executable #1689
Conversation
I'm on board with this 👍 . I think the reason that they are defined in more than one place has to do with preventing circular dependencies, though I'm neither happy with the existence of this problem or its (current) solution.
I'm not sure, but @andyneff might have a better idea 👍 |
sschuberth
changed the title
|
While looking more into this, I realized we should probably also add proper version information to the Windows executable (and an icon, while at it). My final goal would be to create the official Windows distribution on CI to be fully reproducible, and not on some "blessed" developer's machine. |
sschuberth
changed the title
|
Ok, so I changed the topic of this PR a bit ... in fact it's now not less places where some sort of version number is defined. But at least there now is a canonical place where to bump the version information for the Windows build: In the new |
lfs/lfs.go
Outdated
There was a problem hiding this comment.
I think I'd rather have the version stay here than in config, since it makes more sense to say lfs.Version than config.Version and have to look at your import paths to realize that the config package comes from github.com/git-lfs/git-lfs.
There was a problem hiding this comment.
I can do that. It also solves the issue of Version being a var in config/version.go. IMO it should be a const like it is here.
There was a problem hiding this comment.
Turns out that when I import github.com/git-lfs/git-lfs/lfs in config/version.go in order to use lfs.Version I get import cycle not allowed :-( Any suggestion how to solve that?
There was a problem hiding this comment.
Ah, that's because the github.com/git-lfs/git-lfs/lfs package already has a dependency pointing towards github.com/git-lfs/git-lfs/config, introducing another dependency the other way around would be a circular dependency issue. Let's just leave it in the config package for now, even though it's kind of unfortunate.
Untangling that stuff would be more time than I think it's worth at this stage.
There was a problem hiding this comment.
FWIW I think it should eventually in ./git-lfs.go or something:
import gitlfs "github.com/git-lfs/git-lfs"
import "fmt"
func main() {
fmt.Println(gitlfs.Version)
}Oh god, I just realized git-lfs is not a valid go package. What have we donnnnnne.
As far as this PR is concerned, config.Version is just fiiiine 👍
There was a problem hiding this comment.
BTW, I was wondering why the version still is 1.5.0 in master. Don't you merge back the release branches to master?
There was a problem hiding this comment.
Nope, release-1.5 is basically its own thing. Bug fixes will have to be backported to it. The master branch is diverging a lot already, with the scanner refactoring PRs.
I suppose we should change it to 2.0.0-dev or something though.
There was a problem hiding this comment.
Is this a part of the LFS version, or a version for the .rc format?
There was a problem hiding this comment.
It's part of the LFS version. I'm not super happy about the syntax either, but that's how it is :-/
There was a problem hiding this comment.
Does this need to do anything with git-lfs-x64.exe too?
There was a problem hiding this comment.
Nope, I arbitrarily chose the x86 version. As both executables have the version information embedded it does not matter which one Inno Setup is looking at here. But I should make that clearer by adding a comment.
|
I made a few minor updates, but I still wonder about how to conveniently update the version numbers across all places we now have. Should I add a shell script that runs |
|
We basically just look at previous releases to see what needs to change. I don't think a script is necessary for this PR, but would be welcome. |
I've rebased the PR and added such a script. |
There is no reason for it to be variable.
…table This fixes the display of the application icon in Windows' uninstall dialog and allows Inno Setup to use the version information from the executable. Note that AppVeyor has windres.exe in all these locations: C:\cygwin\bin\windres.exe C:\cygwin64\bin\windres.exe C:\MinGW\bin\windres.exe C:\mingw-w64\i686-5.3.0-posix-dwarf-rt_v4-rev0\mingw32\bin\windres.exe C:\msys64\mingw32\bin\windres.exe C:\msys64\mingw64\bin\windres.exe C:\msys64\usr\bin\windres.exe C:\Qt\Tools\mingw482_32\bin\windres.exe C:\Qt\Tools\mingw491_32\bin\windres.exe C:\Qt\Tools\mingw492_32\bin\windres.exe C:\Qt\Tools\mingw530_32\bin\windres.exe C:\Ruby193\DevKit\mingw\bin\windres.exe C:\Ruby23\DevKit\mingw\bin\windres.exe C:\Ruby23-x64\DevKit\mingw\bin\windres.exe But only the Ruby versions work without giving an "preprocessing failed" error.
|
Looks like this is again waiting forever for CircleCI to return its result. Is this a known issue? |
We had some issues last week where we'd exceeded our allotted build time for the month, but that issue has since been resolved. I'm not seeing any other weird changes to the |
Well, I did that just a few minutes ago, but I certainly can try again ... |
|
I don't see my PR being built at https://circleci.com/gh/git-lfs/git-lfs, so maybe we're indeed out of build time again? |
Hm, doesn't seem like it: the last PR I just pushed is building now. Going to loop in @technoweenie since he owns the Circle account. |
This is done in a separate commit to check whether this makes trigger Circle CI properly.
|
Circle CI still is "Waiting for status to be reported", @technoweenie could you please advise? |
|
@sschuberth Our Circle trial expired, but we updated the billing recently. Can you update the branch with the latest from master, to kick the jobs off again? I'd do it, but you posted this on your fork. |
|
@technoweenie How recently did you update the billing? My last change to this PR is from 6 hours ago, but still Circle CI did not seem to kick off. |
|
Hmm... We're at 18% of our allotted build minutes. No errors from Circle CI. SHRUG. Just going to merge it, it's not like this PR adds anything remotely risky from mac builds. |
|
Thanks! |
3 participants
I'm currently looking into reducing the number of places where the version is defined, so that less places need to be touched when bumping the version for a new release, like e.g. in 4e5ba41.
But I have a few questions in this regard. To start with, where are the values for the variables in this line of the RPM spec defined? Do these come from a few lines above? I'm asking because these definitions differ in the case from the variable use.