Lf: Add Go vendoring

[ptzz]

I just tried to make a Homebrew package for mac, but run into this when validating the package:

go_resource`s are deprecated. Please ask upstream to implement Go vendoring

I'm not into go but guess it's about making go dependencies explicit?

[ptzz]

From https://github.com/Homebrew/homebrew-core/pull/29393#issuecomment-399875874:

You can recommend they use dep, glide, govendor (and/or a vendor directory checked into source control).

[gokcehan]

Thanks for working on this package @ptzz.

As far as I know, vendoring is about being explicit about the versions of dependencies so the build does not break whenever a dependency pushes a breaking change. I haven't got much interested in this since these are mostly non-issues on the scale of lf. We have only two library dependencies (termbox-go and go-runewidth) which are both pretty stable by now. Since there is no active development they don't do versioned releases either. I don't think we have experienced a broken build because of a dependency update so far.

The other motivation is to have reproducible builds, which I think why homebrew requires this. However, go team has been working on vgo for this problem and it is likely to be merged to the official go toolchain in the upcoming days. So I think it is a bad time to adopt one of these tools they mention. Also maybe it is a bad time for them to deprecate that feature you are trying to use. If it is just a warning, can you maybe leave it as it is for now?

Also, does homebrew support binary packages? We already have binaries for macos in our releases section. I feel like this would benefit more to the users since they would not need to have go to install lf.

[ptzz]

Unfortunately it's a hard requirement for new formulas. The PR doesn't pass their tests.

They do support binary packages, but afaik they build those themselves from source and require all formulas to be source-buildable.

They mentioned dep, which looks like it's also a go team tool. That will eventually be replaced by vgo then I assume?

[gokcehan]

dep seems to be the official experiment but they also mention that vgo is independent of dep and may not be compatible. I really don't know much to have an opinion about these.

In the wiki, it says vgo will be merged as an experimental feature in Go 1.11 which is due July 31. They recommend using dep or migrating to it for now. I guess we can temporarily use dep as well. This may or may not worth the effort. I worry dep may mess with our travis builds. Also I hope there is a way to get away without including other repositories in our repository.

[ilovezfs]

Also I hope there is a way to get away without including other repositories in our repository.

Only Gopkg.lock and Gopkg.toml need to be checked into your repository to use dep.

Then in the formula we run

system "dep", "ensure", "-vendor-only"

See for example https://github.com/bcicen/slackcat/commit/1ed715826fb36b7ea74b74dc07ac5bcd635026fa

[gokcehan]

Ok so I have added support for dep. It seems that travis does not change anything if an install rule is defined. I have added vendor directory to gitignore. Can you give it a try and report back if there is a problem?

[ilovezfs]

@gokcehan nice! It works.

[gokcehan]

@ilovezfs Glad to hear that. Don't forget to add the package to packages wiki if it is working already. I will keep this issue open for now in case we change to vgo later on.

[ilovezfs]

@gokcehan we'll need a new tag for https://github.com/Homebrew/homebrew-core/pull/29393

[ptzz]

I've pushed an update to https://github.com/Homebrew/homebrew-core/pull/29393 and tests pass now. Let's see if they have further comments. As @ilovezfs pointed out they probably require a new release.

[ilovezfs]

@ptzz just fyi I'm the lead maintainer of https://github.com/Homebrew/homebrew-core

[gokcehan]

@ilovezfs Thanks for helping with this. We haven't changed anything in lf since our last release. I have a few things in mind to add in the upcoming days. We can then make a new release.

[gokcehan]

@ilovezfs @ptzz By the way, if you're using releases for the formulas, can you also maybe pass the version string while building? People like to see versions when they are using lf -version otherwise it can get confusing (related #99). You can see an example in xbuild.sh. There you can also see how to make it a static (i.e. CGO_ENABLED=0) and stripped (i.e. -ldflags="-s -w") binary which could also be nice. I think the full building command should be something like:

env CGO_ENABLED=0 go build -ldflags="-s -w -X main.gVersion=$(git describe --tags)"

Also maybe it may be better to use r5 instead of 5 for the version in the formula.

[ilovezfs]

@gokcehan ideally you'd use a semver. r5 is an extremely unusual version number and won't be parsed correctly automatically from the url. To wit,

Josephs-MacBook-Pro:~ joe$ brew irb
==> Interactive Homebrew Shell
Example commands available with: brew irb --examples
irb(main):001:0> Version.parse("https://github.com/gokcehan/lf/archive/r5.tar.gz")
=> #<Version:0x00007fbc628eddb8 @version="5">
irb(main):002:0> 

and it's very inconvenient to need to manually set an override

version "r5"

on every upgrade.

Regarding CGO_ENABLED=0, -s, -w, etc. it would be best if you could provide a Makefile if you want any ldflags beyond the version string set.

[gokcehan]

@ilovezfs Sorry for giving you trouble about the version. I thought about semver but we don't do major breaking changes and bug fix releases, so it would have 2 redundant numbers for us. Hence we opted for serial versioning instead. I think it is fine if it parses the version as 5. I just thought it might have been accidental.

I have now added a build script for a static stripped binary with version information. You can use it from the root as:

gen/build.sh

You can also pass parameters to go build to give an output path:

gen/build.sh -o bin/lf

[ilovezfs]

@gokcehan Thanks! We use the source archive tarball not a full Git clone, so the git describe won't work. Can you allow the build script to accept a version string parameter too please?

[gokcehan]

I have now added an optional version parameter to build scripts. You should be able to use it as follows:

version=r5 gen/build.sh

[ilovezfs]

@gokcehan yes that works, although you forgot to set the execution bit on the script. I'd still suggest you go with 5.0.0 even if the last two digits are never non-zero as x.y.z is pretty universal and the r isn't communicating anything.

[gokcehan]

@ilovezfs These script files have permission 0755 on my machine. I have also tried downloading the the repository as zip and the execution bit seems to be set correctly. Can you make sure this is not a problem with your system?

For the versioning, we may as well change to semver at some point if it starts to become a major problem but it does not seem to be urgent for now. By the way, I adopted this scheme from three.js in case you are wondering, though I agree that it is uncommon. I like the idea of having a single scheme for versioning but I don't think semver is flexible enough for this purpose. I'm sure semver is appropriate for many projects but different projects have different needs. For example, date based versioning makes much more sense for some projects (e.g. ubuntu 18.04). For most projects that have a stable development, two digit seems to be enough. Major release numbers even in huge projects like linux kernel for example is sometimes meaningless. There are many popular projects that operate with two digits versioning such as tmux. There are also many projects that often misuse semver which shows that it is sometimes inadequate.

The issue here is that versions sometimes need to be easily recognizable such as when implementing parsing logic. For this purpose, I think python's pep 440 serves as a better authority compared to choosing a single scheme like semver. In our case, I agree that r does not communicate anything. It is only there as an equivalent to having a v prefix in version strings as a short for version. I think it is fine to call the version without the r prefix as well.

[ilovezfs]

These script files have permission 0755 on my machine. I have also tried downloading the the repository as zip and the execution bit seems to be set correctly. Can you make sure this is not a problem with your system?

You're correct. I think the problem is that brew applies the patches using the patch -g 0 -f -p1 -i command, which apparently doesn't respect the permissions settings for file creation. TIL.

[ilovezfs]

Yeah from the patch man page

       Context diffs cannot reliably represent the  creation  or  deletion  of
       empty  files,  empty  directories,  or  special  files such as symbolic
       links.  Nor can they represent changes to file metadata like ownership,
       permissions, or whether one file is a hard link to another.  If changes
       like these are also  required,  separate  instructions  (e.g.  a  shell
       script) to accomplish them should accompany the patch.

[ilovezfs]

@gokcehan I've pushed an update to https://github.com/Homebrew/homebrew-core/pull/29393 if you'd like to take a look.

[gokcehan]

@ilovezfs It seems good to me, thanks, though I don't have a machine to test it. I should make a new release as soon as possible to simplify your work.

[gokcehan]

@ilovezfs @ptzz I have released r6 now.

[ilovezfs]

@gokcehan Awesome! Thanks. I've just shipped lf 6 in Homebrew!

[gokcehan]

@ilovezfs @ptzz Thanks again for the package. I'm closing this issue now. No need to track the progress of vgo here.