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

goredo 0.12.3 (new formula) #68846

Closed
wants to merge 1 commit into from
Closed

Conversation

kaihendry
Copy link

  • 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>)?

@BrewTestBot BrewTestBot added go Go use is a significant feature of the PR or issue new formula PR adds a new formula to Homebrew/homebrew-core labels Jan 12, 2021
@kaihendry kaihendry changed the title Package http://www.goredo.cypherpunks.ru/Install.html goredo 0.11.0 (new formula) Jan 12, 2021
class Goredo < Formula
desc "Go implementation of djb's redo, a Makefile replacement that sucks less"
homepage "http://www.goredo.cypherpunks.ru/"
url "http://www.goredo.cypherpunks.ru/download/goredo-0.11.0.tar.zst"
Copy link
Member

Choose a reason for hiding this comment

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

Is there no tar.gz?

Copy link
Author

Choose a reason for hiding this comment

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

Correct, only zstd http://www.goredo.cypherpunks.ru/Install.html

Used by default in Arch and Fedora IIUC

goredo_prefix = "goredo-#{version}"
system "tar", "--use-compress-program", "unzstd", "-xvf", "#{goredo_prefix}.tar.zst"
cd goredo_prefix do
ENV["GOPATH"] = Pathname.pwd
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be needed with modern Go programs AFAIK.

Copy link
Author

Choose a reason for hiding this comment

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

It is when you distribute it with all its dependencies like Sergey has chosen to do.

No need to fetch dependencies from the Internet.

Copy link
Member

Choose a reason for hiding this comment

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

golang/go#30329

So yes: the current plan is to deprecate GOPATH in Go 1.13 (scheduled for August 2019).

Copy link
Author

Choose a reason for hiding this comment

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

TIL! With the help of the author Sergey, we've migrated to Go modules.

system "go", "build", *std_go_args, "go.cypherpunks.ru/goredo"
end
cd bin do
system "./goredo", "-symlinks"
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean there is a definition of what to symlink defined in bin?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure what you're are saying, it basically a dist helper function to create the various redo-* symlinks to the binary.

Copy link
Member

Choose a reason for hiding this comment

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

Where does the definition about those symlinks live?

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I was expecting there to be some definition file. But if it's hardcoded I guess that's fine.

Copy link
Author

Choose a reason for hiding this comment

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

So no changes required? I'm not sure what the next steps are. Do I mark the conversations are resolved? Not sure what to make of the "Resolve conversation" button.

end

test do
assert_match version.to_s, shell_output("#{bin}/redo-ifchange --version")
Copy link
Member

Choose a reason for hiding this comment

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

We need a test that exercises the some of the functionality of the app. Version checks or usage checks (foo --version or foo --help) are not sufficient, as explained in the formula cookbook.

In most cases, a good test would involve running a simple test case: run #{bin}/foo input.txt.

  • Then you can check that the output is as expected (with assert_equal or assert_match on the output of shell_output)
  • You can also check that an output file was created, if that is expected: assert_predicate testpath/"output.txt", :exist?

Some advice for specific cases:

  • If the formula is a library, compile and run some simple code that links against it. It could be taken from upstream's documentation / source examples.
  • If the formula is for a GUI program, try to find some function that runs as command-line only, like a format conversion, reading or displaying a config file, etc.
  • If the software cannot function without credentials, a test could be to try to connect with invalid credentials (or without credentials) and confirm that it fails as expected.
  • Same if the software requires a virtual machine, docker instance, etc. to be running.

Copy link
Author

Choose a reason for hiding this comment

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

I added a test:

  • redo command existence
  • -no-progress -- tests that it is goredo implementation, having that option
  • ability of .do file searching
  • ability of .do file running with the working shell (if OS does not have /bin/sh, then it will fail, as an example)

@carlocab carlocab added the CI-force-arm [DEPRECATED] Don't pass --skip-unbottled-arm to brew test-bot. label Jan 12, 2021
@kaihendry kaihendry changed the title goredo 0.11.0 (new formula) goredo 0.12.0 (new formula) Jan 13, 2021
@kaihendry kaihendry changed the title goredo 0.12.0 (new formula) goredo 0.12.1 (new formula) Jan 13, 2021
@carlocab
Copy link
Member

Your goredo -symlinks step is failing on ARM:

==> ./goredo -symlinks
panic: runtime error: index out of range [0] with length 0

goroutine 1 [running]:
lukechampine.com/blake3.init.0()
	lukechampine.com/[email protected]/cpu_darwin.go:20 +0xfc

@kaihendry kaihendry changed the title goredo 0.12.1 (new formula) goredo 0.12.3 (new formula) Jan 14, 2021
carlocab
carlocab previously approved these changes Jan 14, 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 for your first contribution to homebrew/core, @kaihendry! I hope it's the first of many.

@BrewTestBot
Copy link
Member

:shipit: @carlocab has triggered a merge.

@BrewTestBot
Copy link
Member

⚠️ @carlocab bottle publish failed.

@BrewTestBot BrewTestBot dismissed carlocab’s stale review January 14, 2021 14:30

bottle publish failed

@BrewTestBot
Copy link
Member

:shipit: @carlocab has triggered a merge.

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Feb 14, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Feb 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI-force-arm [DEPRECATED] Don't pass --skip-unbottled-arm to brew test-bot. go Go use is a significant feature of the PR or issue new formula PR adds a new formula to Homebrew/homebrew-core outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants