Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vcsh 2.0.2 #84672

Closed
wants to merge 1 commit into from
Closed

vcsh 2.0.2 #84672

wants to merge 1 commit into from

Conversation

RichiH
Copy link
Contributor

@RichiH RichiH commented Sep 4, 2021

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • 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 --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

I don't use OS X nor homebrew, I am upstream. I modeled this PR on what others did, but did it by hand. Should be good to go.

@BrewTestBot BrewTestBot added the deprecated license Formula uses a deprecated SPDX license which should be updated label Sep 4, 2021
@alerque
Copy link
Contributor

alerque commented Sep 4, 2021

This is going to need a little help, just bumping the versions isn't enough because the previous iteration of the formula did not use autotools. We need to find something that does and figure out the right macros to trigger ./configure and make install with Homebrew's presets.

@alerque
Copy link
Contributor

alerque commented Sep 4, 2021

This is also the wrong source, you should be pulling the sdist attached to the release, not the git Archive. Less tooling is needed to build the sdist.

@alerque
Copy link
Contributor

alerque commented Sep 4, 2021

I don't have system with Homebrew to test this one, but here is a patch that will at least get this closer:

From 662c118a4d630440bcb24490cc20f485933ff989 Mon Sep 17 00:00:00 2001
From: Caleb Maclennan <caleb@alerque.com>
Date: Sat, 4 Sep 2021 22:42:29 +0300
Subject: [PATCH] vcsh: use distribution source, update autotools install

Signed-off-by: Caleb Maclennan <caleb@alerque.com>
---
 Formula/vcsh.rb | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/Formula/vcsh.rb b/Formula/vcsh.rb
index f9d219ec112..9d52361d188 100644
--- a/Formula/vcsh.rb
+++ b/Formula/vcsh.rb
@@ -1,19 +1,28 @@
 class Vcsh < Formula
   desc "Config manager based on git"
   homepage "https://github.com/RichiH/vcsh"
-  url "https://github.com/RichiH/vcsh/archive/refs/tags/v2.0.1.tar.gz"
-  version "2.0.1"
-  sha256 "470acd9a0b7d60439fec151ce88a9de92845c1f0a9ecc6868160fa82569cd3e1"
+  url "https://github.com/RichiH/vcsh/releases/download/v2.0.1/vcsh-2.0.1.tar.xz"
+  sha256 "2769e01c8f2fcdaa32cc9f48e86124f3d7c2e93465c75eff33634fa22a86da68"
   license "GPL-2.0"
 
   bottle do
-    sha256 cellar: :any_skip_relocation, all: "d6adf58cb690fa63602d8056d347bf55dde6ebd090c6f82d1bbbbc78b137d93b"
+    sha256 cellar: :any,                 arm64_big_sur: "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
+    sha256 cellar: :any,                 big_sur:       "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
+    sha256 cellar: :any,                 catalina:      "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
+    sha256 cellar: :any,                 mojave:        "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
+    sha256 cellar: :any_skip_relocation, x86_64_linux:  "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
   end
 
+  uses_from_macos "git"
+
   def install
-    bin.install "vcsh"
-    man1.install "vcsh.1"
-    zsh_completion.install "_vcsh"
+
+    args = %W[
+      --prefix=#{prefix}
+    ]
+
+    system "./configure", *args
+    system "make", "install"
   end
 
   test do
-- 
2.33.0

There may be more to do, but hopefully this gets things moving in the right direction.

You can add me to your Homebrew-core fork if you want me to apply and experiment with this branch.

@carlocab carlocab changed the title Update vcsh to 2.0.1 vcsh 2.0.1 Sep 4, 2021
@carlocab carlocab mentioned this pull request Sep 4, 2021
6 tasks
@carlocab
Copy link
Member

carlocab commented Sep 4, 2021

Unfortunately make install fails with

cp -bf completions/vcsh.bash completions/vcsh
cp -bf completions/vcsh.zsh completions/_vcsh
cp: illegal option -- b
usage: cp [-R [-H | -L | -P]] [-fi | -n] [-apvXc] source_file target_file
       cp [-R [-H | -L | -P]] [-fi | -n] [-apvXc] source_file ... target_directory

We could perhaps use coreutils during the build to provide cp, but it would be nice if we could make the Makefile portable.

@carlocab carlocab added the build failure CI fails while building the software label Sep 4, 2021
@alerque
Copy link
Contributor

alerque commented Sep 4, 2021

I can make a patch release upstream so the Makefile is more portable. Portability and ease of distro packaging across platforms was a major point of this release cycle. In this case the -b is completely unnecessary. Is the -f portable? The -b flag was to avoid interactive prompts when rebuilding the same source a second time while still assuring the file got updated if needed. I swapped out cp -bf for install which should do the same thing by default across platforms.

@alerque
Copy link
Contributor

alerque commented Sep 4, 2021

v2.0.2 should have a quick fix for that portability issue.

@carlocab carlocab force-pushed the patch-1 branch 3 times, most recently from 775f7be to 761332d Compare September 5, 2021 04:17
@carlocab carlocab added CI-force-linux [DEPRECATED] Don't pass --skip-unbottled-linux to brew test-bot. audit failure CI fails while auditing the software and removed build failure CI fails while building the software CI-force-linux [DEPRECATED] Don't pass --skip-unbottled-linux to brew test-bot. labels Sep 5, 2021
    Signed-off-by: Richard Hartmann <richih@richih.org>
    Co-authored-by: Caleb Maclennan <caleb@alerque.com>
    Co-authored-by: Carlo Cabrera <30379873+carlocab@users.noreply.github.com>
@carlocab carlocab changed the title vcsh 2.0.1 vcsh 2.0.2 Sep 5, 2021
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @RichiH, @alerque.

@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

system "make", "install"

# Remove references to git shim
inreplace bin/"vcsh", %r{#{HOMEBREW_SHIMS_PATH}/[^/]+/super/git}o, "git"
Copy link
Contributor

@alerque alerque Sep 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the help with this @carlocab. I do have a question about this line though. Are we doing something wrong upstream detecting the path to git at configure time? Or is this to correct for an idiosyncrasy in Homebrew where the compile time path to Git is different than what should be used at run time?

Either way I think there is a better way to do this. If the configure time detection (that uses autotools' built in program detection) is not turning up the correct answer, you can inform it manually with ./configure GIT=/path/to/git or even ./configure GIT=git if using relative paths at runtime is appropriate for the platform. This goes for other tools that are detected as well. That's what bothers me here, if Git is not being detected correctly are other tools?

Also I'm curious why there is no depends_on "git" or uses_from_macos "git" or similar. Shouldn't there be some sort of dependency declared for this to work reliably?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On is this to correct for an idiosyncrasy in Homebrew where the compile time path is different than what should be the run time path?

Yes, this, at least in the case of Git, and possibly any absolute path on Linux. Is there any danger in querying PATH at runtime instead of hardcoding absolute paths into vcsh? It seems to me that the former is more flexible, but perhaps there's a reason you're doing this I don't see.

That's what bothers me here, if Git is not being detected correctly are other tools?

The audit should catch stuff that's definitely wrong, though I'm a bit wary of hardcoding /bin/sed and /bin/grep as the paths to search for these on Linux at runtime.

Also I'm curious why there is no depends_on "git" or uses_from_macos "git" or similar. Shouldn't there be some sort of dependency declared for this to work reliably?

Homebrew itself won't work without Git, so any system with Homebrew installed will also have a system Git installation. Doing depends_on "git" or uses_from_macos "git" means we want to use the git formula (and the latter only on Linux); usually because we need a new enough version of Git for things to work. Is that the case here?

@github-actions github-actions bot added the outdated PR was locked due to age label Oct 6, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
audit failure CI fails while auditing the software CI-force-linux [DEPRECATED] Don't pass --skip-unbottled-linux to brew test-bot. deprecated license Formula uses a deprecated SPDX license which should be updated outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants