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

termsql 0.3 (new formula) #7801

Closed
wants to merge 4 commits into from
Closed

termsql 0.3 (new formula) #7801

wants to merge 4 commits into from

Conversation

kaeff
Copy link

@kaeff kaeff commented Dec 12, 2016

Hi,

I'd like to propose this new formula. I'm not the package author, nor experienced with writing formulas for python projects, but I've made sure to follow the docs as best as I can. Happy for your feedback, thanks in advance!

cf. tobimensch/termsql#3


def install
# Replace path to man pages in hard-coded setup.py
new_contents = File.read("setup.py").gsub(%r{\/usr\/share\/man\/man1\/}, man1)
Copy link
Member

Choose a reason for hiding this comment

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

This can be done with inreplace instead, to avoid separate read/writes:

inreplace "setup.py" do |s|
  s.gsub! %r{/usr/share/man/man1/}, man1
end

You should also report it upstream (build parameters never hurt 😉)

class Termsql < Formula
desc "Convert text into SQL table and query it"
homepage "https://tobimensch.github.io/termsql/"
url "https://github.com/tobimensch/termsql.git", :using => :git
Copy link
Member

Choose a reason for hiding this comment

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

Should be fine to leave :using => :git off due to the .git suffix.

@woodruffw woodruffw added the new formula PR adds a new formula to Homebrew/homebrew-core label Dec 14, 2016
desc "Convert text into SQL table and query it"
homepage "https://tobimensch.github.io/termsql/"
url "https://github.com/tobimensch/termsql.git", :using => :git
version "0.3"
Copy link
Member

Choose a reason for hiding this comment

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

Where is this version coming from? I can't find any tags/releases on upstream.

@woodruffw
Copy link
Member

Thanks for taking the time to contribute @kaeff! I've left some comments for you to look over, let me know if you have any questions.

@kaeff
Copy link
Author

kaeff commented Dec 14, 2016

Thanks @woodruffw !

I asked @tobimensch about adding a version tag and making the man1 path configurable here: tobimensch/termsql#3 . I've guessed the version from running termsql -v after installing. Is that an acceptable workaround?

@woodruffw
Copy link
Member

Thanks for raising the issues with upstream.

Unfortunately, getting the version from -v isn't an acceptable workaround -- Homebrew needs some kind of immutable tag or archive (and corresponding sha256). If this wasn't the case, any upstream could break their OS X build at any point and we would be caught fielding support for them.

@kaeff
Copy link
Author

kaeff commented Dec 14, 2016

How about using the commitish as the version?

@woodruffw
Copy link
Member

Unfortunately, no. The version should be a stable release put out by upstream.

@tobimensch Could you put out either a tag or a full release for termsql? That's the main blocker on this formula.


def install
# Replace path to man pages in hard-coded setup.py
inreplace "setup.py", %r{/usr/share/man/man1/}, man1
Copy link
Contributor

Choose a reason for hiding this comment

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

man1 doesn't have a trailing slash, so this looks sketchy to me

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't be necessary according to the spec (https://docs.python.org/2/distutils/setupscript.html#installing-additional-files), but we can append a slash anyway for consistency.

Copy link
Contributor

Choose a reason for hiding this comment

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

@woodruffw What’s @ilovezfs is saying is that this line replaces something that ends with a slash with something that doesn’t.

Copy link
Member

Choose a reason for hiding this comment

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

@bfontaine I think I understood that, I guess I'm not clear on what the problem is. Looking at the file itself:

data_files=[('/usr/share/man/man1/', ['termsql.1.gz'])]

should be changed to something like this:

data_files=[('/usr/local/Cellar/termsql/0.3/share/man/man1', ['termsql.1.gz'])]

There's no trailing slash there, but it shouldn't matter to setup.py according to the spec above. Please correct me if I'm misunderstanding something!

Copy link
Member

@MikeMcQuaid MikeMcQuaid Dec 18, 2016

Choose a reason for hiding this comment

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

Let's use "#{man1}/" here, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

@woodruffw Thanks, that’s clear. I didn’t look at the file itself and thought we might replace e.g. "/usr/share/man/man1/foo.1" with "#{man1}foo.1".


test do
# Run an example query (count processes)
system "ls -lha /usr/bin/* | termsql -w -r 8 \"select count(*) from tbl\""
Copy link
Member

Choose a reason for hiding this comment

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

Use pipe_input here instead of a manual pipe and assert_match on the output.

@kaeff
Copy link
Author

kaeff commented Jan 2, 2017

@woodruffw @MikeMcQuaid I've addressed your requested changes. Could you have another look? The upstream tag is still missing unfortunately.

version "22501e5c982ec04229643802e36756feb4687e89"
sha256 "d8fabe96f57153a2e2852995d66f4506cf593bae4a6b9d7d76d3aef6150fb07e"

depends_on :python => ["sqlite3", "sqlparse"]
Copy link
Member

Choose a reason for hiding this comment

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

These should be vendored instead e.g. like ansible.rb

desc "Convert text into SQL table and query it"
homepage "https://tobimensch.github.io/termsql/"

url "https://github.com/tobimensch/termsql/archive/22501e5c982ec04229643802e36756feb4687e89.tar.gz"
Copy link
Member

Choose a reason for hiding this comment

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

Still need an upstream tag before we can include this, I'm afraid.

@dunn dunn added the needs response Needs a response from the issue/PR author label Jan 7, 2017
@fxcoudert
Copy link
Member

Closing for now, please reopen if circumstances change.

@fxcoudert fxcoudert closed this Mar 7, 2017
@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs response Needs a response from the issue/PR author new formula PR adds a new formula to Homebrew/homebrew-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants