Skip to content
This repository has been archived by the owner on Jul 4, 2023. It is now read-only.

squid-deb-proxy 0.8.14 (new formula) #48854

Closed
wants to merge 5 commits into from
Closed

squid-deb-proxy 0.8.14 (new formula) #48854

wants to merge 5 commits into from

Conversation

Elemecca
Copy link

@Elemecca Elemecca commented Feb 5, 2016

This adds a new formlua for squid-deb-proxy, a caching HTTP proxy for Debian / Ubuntu APT repositories based on Squid. It's useful to have on OS X if e.g. you spin up Debian-based VMs frequently with Vagrant.

This adds a new formlua for [squid-deb-proxy], a caching HTTP proxy for
Debian / Ubuntu APT repositories based on Squid. It's useful to have on
OS X if e.g. you spin up Debian-based VMs frequently with Vagrant.

[squid-deb-proxy]: https://launchpad.net/squid-deb-proxy
@Elemecca
Copy link
Author

Elemecca commented Feb 5, 2016

Ah, damn, that generated tarball isn't stable.

The current source URL, an auto-generated tarball from Launchpad's UI
for Bazaar, apparently isn't generated identically every time, which was
causing checksum failures. This switches it out for the orig tarball
from Ubuntu's APT repository.
@Elemecca Elemecca changed the title squid-deb-proxy 0.8.13 (new formula) squid-deb-proxy 0.8.14 (new formula) Feb 5, 2016
@DomT4
Copy link
Member

DomT4 commented Feb 6, 2016

Use Debian:

url "https://mirrors.ocf.berkeley.edu/debian/pool/main/s/squid-deb-proxy/squid-deb-proxy_0.8.14.tar.gz"

class SquidDebProxy < Formula
desc "APT repo caching proxy based on Squid."
homepage "https://launchpad.net/squid-deb-proxy"

Copy link
Member

Choose a reason for hiding this comment

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

Don't need this spacing.

- fix rubocop warnings about parentheses with heredocs
- clean up extra spacing and comments
- use Debian's mirrors instead of Ubuntu's
- use "#{sbin}" instead of "#{HOMEBREW_PREFIX}/opt/squid/sbin"
@Elemecca
Copy link
Author

Elemecca commented Feb 8, 2016

OK, I've addressed most of the review comments.

I'm not sure about the Pathname thing, though. I replaced Pathname.read and Pathname.write with IO.read and IO.write the first time they were used because it didn't make any real difference. However, in all the other instances of Pathname I'm using it because Homebrew has extended Pathname.write to automatically create the parent directory, whereas IO.write just fails if it doesn't exist. I can switch to explicitly calling mdir_p( dirname( path ) ) before IO.write, but I don't feel that using Pathname is wrong there since it actually is doing path manipulation.

Also, out of curiosity, why the switch to the Debian mirror, especially since squid-deb-proxy appears to be an Ubuntu project (at least, it's hosted on Launchpad)?


# additional installation from debian/squid-deb-proxy.install
mkdir_p "#{prefix}/usr/share/squid-deb-proxy"
cp "init-common.sh", "#{prefix}/usr/share/squid-deb-proxy/"
Copy link
Author

Choose a reason for hiding this comment

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

This is a bit odd. What is it doing?

For some reason the upstream project, which is intended to be a Debian package, installs this file using the Debian packaging metadata instead of the install target of the Makefile. I therefore have to install it explicitly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you ask the developers if they'd consider adding it to the Make target?

Also, those two lines can be one:

(prefix/"usr/share/squid-deb-proxy").install "init-common.sh"

Changing from "#{HOMEBREW_PREFIX}/opt/squid/sbin/squid" to
"#{sbin}/squid" makes the launcher look for `squid` in
"/usr/local/Cellar/squid-deb-proxy-0.8.14/sbin/squid" where it very much
isn't. This reverts that change.
@DomT4
Copy link
Member

DomT4 commented Feb 9, 2016

Also, out of curiosity, why the switch to the Debian mirror, especially since squid-deb-proxy appears to be an Ubuntu project (at least, it's hosted on Launchpad)?

I presumed this comment:

Ah, damn, that generated tarball isn't stable.

Was applying to the current URL, which was Launchpad at the time. If that's not the case, use LP by all means.

cp "init-common.sh", "#{prefix}/usr/share/squid-deb-proxy/"

# produce execution wrapper script
Pathname.new("#{sbin}/squid-deb-proxy").write(
Copy link
Member

Choose a reason for hiding this comment

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

Don't need to call Pathname here...

(sbin/"squid-deb-proxy").write <<-EOS.undent
  #!/bin/bash

  example
  example 2
  and three
EOS

Should work just fine.

- switch to idiomatic style of Pathname usage
- move `test` block to end of class
- use correct API for reference to squid's `sbin` dir
@Elemecca
Copy link
Author

Elemecca commented Mar 4, 2016

@DomT4 I think I've addressed your comments. Can you re-review? Should I squash down to one commit?

@DomT4
Copy link
Member

DomT4 commented Mar 11, 2016

One commit is always nice! Has the patch been submitted upstream yet?

@bfontaine
Copy link
Contributor

Ping re Dom’s comment?

@apjanke apjanke added this to the Clear out Legacy milestone May 3, 2016
@apjanke apjanke added this to the Clear out Legacy milestone May 3, 2016
@apjanke
Copy link
Contributor

apjanke commented May 3, 2016

Closing this due to inactivity/lack of response. If you'd still like to pursue this, feel free to respond to the previous comments/questions and re-open on the new homebrew/core repo.

@apjanke apjanke closed this May 3, 2016
@Homebrew Homebrew locked and limited conversation to collaborators Jul 10, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants