step: Fixed missing versions in homebrew binaries. #36997

dopey · 2019-02-15T01:40:09Z

Have you followed the guidelines for contributing?
Have you checked that there aren't other open pull requests for the same formula update/change?
Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

Formula/step.rb

dopey · 2019-02-15T08:14:17Z

Thanks for the feedback and guidance @javian! Let me know if the changes check out.

zbeekman · 2019-02-15T17:24:30Z

Formula/step.rb

@@ -3,6 +3,7 @@ class Step < Formula
  homepage "https://smallstep.com"
  url "https://github.com/smallstep/cli/archive/v0.8.5.tar.gz"
  sha256 "b809c4638ffd6d0c3e1bff66d87d9fbea186783735544492c34f35fe1e9f9d79"
+  head "https://github.com/smallstep/cli.git"


@dopey: I'm curious what your reasoning for adding a head url is. Unless it makes sense to do so to improve software quality, we avoid adding heads to formula. If you plan to use it for improving software quality, e.g., in nightly testing to test for installation regressions and issues caused by build dependencies, then I'm all for adding it. However, unless there is a compelling reason that users might want/need head installations, I would prefer that it is removed. In the future we'll be adding very prominent warnings to HEAD installs saying that they are totally unsupported and you're on your own if something breaks.

I actually grabbed that from another Formula because I thought it might be useful. Should have looked up what it was doing. Removed.

In the same vein I saw another formula using this url definition (https://github.com/Homebrew/homebrew-core/blob/master/Formula/goofys.rb#L6-L8) and was wondering if it wouldn't be nicer to switch to that over the tarball. Any drawbacks?

I actually grabbed that from another Formula because I thought it might be useful. Should have looked up what it was doing. Removed.

I like to add add nightly tests to my projects that build using brew install --HEAD <my-package> to ensure that upstream dependencies, or changes to homebrew didn't break the install procedure for the package in question. I support and encourage this approach, because then the package authors/maintainers catch potential breakage of their package when installed via Homebrew. This way, when they mint a new release, they will have been testing the homebrew installation all along.

However, I discourage adding in simply because "some user may want this". If users want development software, they should be comfortable enough with git to grab it for themselves. The exception to this, in my mind, is tooling stuff. I like knowing if stuff, e.g., in nightly GFortran, might break my projects so that I can update them ahead of the next major compiler release.

Makes sense for that use case, and definitely not one I had in mind when I added it. Thanks for the explanation.

In the same vein I saw another formula using this url definition (/Formula/goofys.rb@master#L6-L8) and was wondering if it wouldn't be nicer to switch to that over the tarball. Any drawbacks?

The way you have it now is probably more canonical. I don't recall whether or not brew will do a shallow clone using the tag and hash version scheme. If it doesn't, then grabbing a whole git repo will be slower and more time consuming than just the tarball, and it will take up more disk space. Unless there is a compelling reason to switch, let's leave the version & download as is.

zbeekman · 2019-02-15T19:07:24Z

Formula/step.rb

@@ -21,7 +21,7 @@ class Step < Formula

  def install
    ENV["GOPATH"] = buildpath
-    contents = Dir["{*,.git,.gitignore}"]
+    contents = Dir.glob("*", File::FNM_DOTMATCH) - %w[. ..]
    (buildpath/"src/github.com/smallstep/cli").install contents


@dopey How hard would it be to push this complexity to the build system? i.e., just be able to run system "make", "build" from the default top level build directory? If it's not too difficult, would upstream be amenable to this? I guess it's possible that this could break other package managers, so I understand if that isn't feasible.

The project uses dep to vendor dependencies, and as far as I can tell dep requires that a package be under $GOPATH/src. I'm not super well acquainted with this stuff, but based on some searching it seems that the other workaround (other than copying the whole project into $GOPATH/src - what we're currently doing) would be to do some symlink magic - which I think would be more confusing. Dependency mgmt is scheduled to become a first class citizen in golang soon, so once we replace dep the $GOPATH/src requirement might go away.

Like I said, I'm not super confident with this stuff. If there are examples of other Formulas that use dep and are able to build directly in the buildpath I'd be happy to take their lead.

This seems to be the common pattern

def install ENV["GOPATH"] = buildpath (buildpath/"src/github.com/ssh-vault/ssh-vault").install buildpath.children

so if you grep for buildpath.children there will be multiple examples from what I can tell.

Thanks for the tip! Updated. Lemme know if that's better / easier to understand.

zbeekman

LGTM

@dopey are we good to merge?

dopey · 2019-02-18T22:16:01Z

@zbeekman Good from my end.

dopey · 2019-02-19T00:55:32Z

Actually, hold on labeling this ready for merge please. Just doing some further testing to make sure the versions made it.

zbeekman · 2019-02-19T01:24:48Z

@dopey ok, we'll hold off. Let us know when you've finished testing

dopey · 2019-02-19T01:26:36Z

@zbeekman It seems that the github release tarball doesn't actually contain a .git directory (which makes sense). That means that any git describe --tags .etc commands that I've got in my build pipeline will fail (so once again the versions don't work). If I switch to using:

url "https://github.com/smallstep/cli.git",
      :tag      => "v0.8.5",
      :revision => "8b0b46f10d3be86cbbe72a36703d3bf6648ce8b7"

then the repo is cloned and has the necessary .git directories and the version setting in the build pipeline works correctly. You mentioned before that there should be a compelling reason to switch to this style of url resource definition. Would this qualify?

Is there a work-around that I just don't know of? I could try to git init and update that way, but I don't see any other formulas doing that.

zbeekman · 2019-02-19T01:44:18Z

Is there a work-around that I just don't know of? I could try to git init and update that way, but I don't see any other formulas doing that.

In my projects I try to encode version information in release assets using the export-subst property in .gitattributes. Now, I know that, in the past, we've had issues if maintainers/devs create a tag through the GitHub interface, rather than using the command line, because that causes a dangling object, and then git describe won't include the tag made through the web interface. It sounds like you're relying on git describe so I doubt you are facing that particular issue. After looking around one of my projects, it appears that the archive created on github respects the export-subst attribute.

E.g., in a file I have named .VERSION the contents under version control might look like:

$Format%d$

Then git archive and the tarball produced by github will contain a .VERSION file with the contents:

(tag: 2.4.0)

if I have

.VERSION export-subst

in my .gitattributes file.

The project I use this for has logic that checks if we're in a git working directory and uses git describe to get the version info, and if we're not in a git project, it checks the .VERSION file for version info.

I would recommend taking this approach (using export substitution for versioning when not in an active git repository) however, if the transition/implementation proves too burdensome, I think we can consider switching your URL/Versioning scheme to using git tags and SHAs.

zbeekman · 2019-02-19T01:45:19Z

I meant to leave this link in my previous comment: info on export-subst

dopey · 2019-02-19T02:01:43Z

Thanks for the info! I'll give this a shot tomorrow. We currently use a similar method to the one you described, but w/out the fallback onto a .VERSION file.

dopey · 2019-02-19T23:59:50Z

Hey @zbeekman I've plugged away at this for a few hours and I feel a bit silly, but I may need a bit more guidance.

$Format:%d$ leaves me with something like (<branch>, tag: v0.8.5) or (tag: v0.8.5, <branch>) depending on the arguments given to git archive (the latter is from the tar ball generated by github releases). So the next step is attempting to parse that line for the tag. I can do this with sed:

sed -n "s/^.*tag:\s*\(\S*\)[,)].*$/\1/p" test.txt

but only on gnu sed on linux so it won't work for people trying to build on osx (homebrew). I'd prefer not to have different ways of doing this for different architectures since that would certainly complicate the build pipeline.

Additionally, I searched for ways to have only the tag written to the .VERSION file, as you indicated, but I'm not sure that's possible using vanilla export-subst. This question on stackoverflow recommends smudgeing as an alternative, but it definitely doesn't seem like a preferred method.

I also looked through your repos on github and, while I found a few examples with .VERSION or version.sh, I wasn't able to find the one you mentioned - github search functionality is not my favorite thing.

If you wouldn't mind linking me to your code, assuming it doesn't overcomplicate our build pipeline, I'd be happy to implement it. Otherwise it may be easier to stick with cloning the repo and getting the version from git describe --tags - understanding the version won't work if people try to build directly from the tarball.

zbeekman · 2019-02-20T00:38:08Z

I can do this with sed:
sed -n "s/^.*tag:\s*$\S*$[,)].*$/\1/p" test.txt
but only on gnu sed on linux so it won't work for people trying to build on osx (homebrew).

I assumed, perhaps mistakenly, that you could use regex functionality in golang, or provided by the build system to grab the tag out of the .VERSION file.

Short of this, I would just rely on bash (or throw in the towel and go to the git based formula versioning). To do it with bash you can do something like:

#!/usr/bin/env bash
read -r firstline < .VERSION
last_half="${firstline##*tag: }"
version_string="${last_half%%[,)]*}"
echo "$version_string"

Additionally, I searched for ways to have only the tag written to the .VERSION file, as you indicated, but I'm not sure that's possible using vanilla export-subst. This question on stackoverflow recommends smudgeing as an alternative, but it definitely doesn't seem like a preferred method.

I've never tried this, and from reading the man page, I'm under the impression that smudge may not work for git archive. Especially given the fact that you can execute arbitrary code with a smudge script, I would guess that even if it works locally, it won't work with the archive that GH produces.

TL;DR: If you want to switch to the tag based versioning scheme I'm ok with it. However, I think having your build system or tool use a golang regex library to parse the version file is more elegant, and short of that you can do it in plain old Bash 3, as I show above. The world is your oyster, and I'm content with whichever of these approaches you decide to take.

dopey · 2019-02-22T00:51:25Z

Apologies for the radio silence. It took a bit of time to update the build process with the .VERSION/.gitattributes stuffs (thanks @zbeekman for the tips) and then we found that zsh completion wasn't working. Had to bump the versions for the modified build process in both cli and certificates. Lemme know if this is looking better :)

zbeekman · 2019-02-22T14:49:11Z

@dopey it looks like you may not have updated the SHA256 hashes. Can you please investigate (and try a local build)?

zbeekman · 2019-02-22T14:51:42Z

Other than the SHA256 miss-matches everything else looks good to me.

Looks good but build failure needs to be fixed

zbeekman

Fix bad sha256

zbeekman · 2019-02-22T14:58:31Z

Formula/step.rb

-  url "https://github.com/smallstep/cli/archive/v0.8.5.tar.gz"
-  sha256 "b809c4638ffd6d0c3e1bff66d87d9fbea186783735544492c34f35fe1e9f9d79"
+  url "https://github.com/smallstep/cli/archive/v0.8.6.tar.gz"
+  sha256 "1aad35a78f8b7cfa59ced27131f547174344acf9333cad1d87731cb7b133ec41"


Suggested change

sha256 "1aad35a78f8b7cfa59ced27131f547174344acf9333cad1d87731cb7b133ec41"

sha256 "8eb5099550d510b24ba1e5a53ccd4141e545286274c6abceb712be702d428820"

@zbeekman thanks for reporting this, @dopey won't be back until Monday, he will take care of this.

Just fixed. It's really strange because I tested that formula before I pushed. It's as though the sha changed from underneath me. It's actually not the first time we've seen that happen with a recently built release. dodgy.

If someone yanked a tag, and force pushed a new one with the same name that will happen. The preferred thing to do is to just warn users off a bad tag with big red letters and push a new tag that increments at least the patch or tweak level.

Copy entire contents of directories - was leaving behind .git files. Fix zsh completions.

zbeekman

LGTM, thanks for your patience and continued hard work, @dopey

Contributor resolved issues identified in this review

zbeekman · 2019-02-22T22:38:16Z

gah, the commit message is malformed, but I think that will get updated automagically, if not I'll fix it.

zbeekman · 2019-02-22T22:44:16Z

Thanks for your contribution to Homebrew! 🎉 🥇

Without awesome contributors like you, it would be impossible to maintain Homebrew to the high level of quality users have come to expect. Thank you!!!!

javian previously requested changes Feb 15, 2019

View reviewed changes

Formula/step.rb Outdated Show resolved Hide resolved

Formula/step.rb Outdated Show resolved Hide resolved

Formula/step.rb Outdated Show resolved Hide resolved

Formula/step.rb Outdated Show resolved Hide resolved

dopey force-pushed the step-version-fix branch from 3fd7987 to bca6902 Compare February 15, 2019 08:06

zbeekman reviewed Feb 15, 2019

View reviewed changes

dopey force-pushed the step-version-fix branch from bca6902 to 068221d Compare February 15, 2019 17:58

zbeekman reviewed Feb 15, 2019

View reviewed changes

dopey force-pushed the step-version-fix branch from 068221d to 5b83ef8 Compare February 18, 2019 18:59

zbeekman previously approved these changes Feb 18, 2019

View reviewed changes

zbeekman added do not merge almost there PR is nearly ready to merge labels Feb 19, 2019

dopey force-pushed the step-version-fix branch from 5b83ef8 to 375a1a7 Compare February 22, 2019 00:48

zbeekman suggested changes Feb 22, 2019

View reviewed changes

step: Fixed missing versions in homebrew binaries.

269e29e

Copy entire contents of directories - was leaving behind .git files. Fix zsh completions.

dopey force-pushed the step-version-fix branch from 375a1a7 to 269e29e Compare February 22, 2019 18:13

zbeekman approved these changes Feb 22, 2019

View reviewed changes

zbeekman removed the do not merge label Feb 22, 2019

zbeekman closed this in c3ee74e Feb 22, 2019

lock bot added the outdated PR was locked due to age label Mar 24, 2019

lock bot locked as resolved and limited conversation to collaborators Mar 24, 2019

	sha256 "1aad35a78f8b7cfa59ced27131f547174344acf9333cad1d87731cb7b133ec41"
	sha256 "8eb5099550d510b24ba1e5a53ccd4141e545286274c6abceb712be702d428820"

Uh oh!

step: Fixed missing versions in homebrew binaries. #36997

step: Fixed missing versions in homebrew binaries. #36997

Uh oh!

Conversation

dopey commented Feb 15, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dopey commented Feb 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zbeekman left a comment

Choose a reason for hiding this comment

Uh oh!

dopey commented Feb 18, 2019

Uh oh!

dopey commented Feb 19, 2019

Uh oh!

zbeekman commented Feb 19, 2019

Uh oh!

dopey commented Feb 19, 2019

Uh oh!

zbeekman commented Feb 19, 2019

Uh oh!

zbeekman commented Feb 19, 2019

Uh oh!

dopey commented Feb 19, 2019

Uh oh!

dopey commented Feb 19, 2019

Uh oh!

zbeekman commented Feb 20, 2019

Uh oh!

dopey commented Feb 22, 2019

Uh oh!

zbeekman commented Feb 22, 2019

Uh oh!

zbeekman commented Feb 22, 2019

Uh oh!

zbeekman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zbeekman left a comment

Choose a reason for hiding this comment

Uh oh!

zbeekman commented Feb 22, 2019

Uh oh!

zbeekman commented Feb 22, 2019

Uh oh!

Uh oh!