pacparser 1.3.7 (new formula) #5661
Conversation
| depends_on "python" => :optional | ||
|
|
||
| def install | ||
| system "make", "-j1", "-C", "src", "install", |
There was a problem hiding this comment.
Does this not work with a parallel build?
There was a problem hiding this comment.
No, it doesn't. It's a known issue. I disable that for my Alpine Linux build of this as well.
There was a problem hiding this comment.
Use ENV.deparallelize and add a link to the upstream issue and a comment in Ruby to clarify.
There was a problem hiding this comment.
Done, committed and squished.
aaronhurt
force-pushed
the
new/pacparser
branch
from
46eb600 to
7ed508c
Compare
|
|
||
| test do | ||
| ## functional tests are run as part of the make | ||
| assert_equal version.to_s, shell_output("#{bin}/pactester -v").strip |
There was a problem hiding this comment.
Can this test be modified to do something more substantial than e.g. --version or --help e.g. parse a simple, embedded PAC file? See cmake.rb for an example of an application formula with a good test and tinyxml2.rb for an example of a library formula with a good test. Thanks!
There was a problem hiding this comment.
The upstream package has test cases that are run at the end of the make. Those tests are -pure functional tests and do run through example PAC files. If/when those tests fail the entire build will fail.
There was a problem hiding this comment.
This is not a test of the software compilation but a test that it's been installed by Homebrew properly.
There was a problem hiding this comment.
Then I guess I'm confused as why a simple "it executes" isn't sufficient for this section if all it is testing is that the binary is in the proper location and executes? Like I said there are a series of actual functional tests run during the make process. I could replicate those in the test section of the package but it seems a bit redundant to run them twice.
There was a problem hiding this comment.
We've found --version is often not sufficient to identify packaging bugs compared to a functional/integration test. Duplicating a single functional test here would be 👍
| # Disable parallel build due to upstream concurrency issue. | ||
| # https://github.com/pacparser/pacparser/issues/27 | ||
| ENV.deparallelize | ||
| system "make", "-j1", "-C", "src", "install", |
There was a problem hiding this comment.
Can remove the -j1 now. Additionally, a Dir.chdir "src" will save needing to repeat it below. You may also want to use a ENV["VERSION"] = version to avoid setting that twice, too.
There was a problem hiding this comment.
No problem, I can change that. Squished and pushed.
aaronhurt
force-pushed
the
new/pacparser
branch
from
5529856 to
135a650
Compare
zmwangx
added
new formula
| class Pacparser < Formula | ||
| desc "Library to parse proxy auto-config (PAC) files" | ||
| homepage "https://github.com/pacparser/pacparser" | ||
| url "https://github.com/pacparser/pacparser/archive/1.3.6.tar.gz" |
There was a problem hiding this comment.
@leprechau Thanks for adding pacparser to homebrew. I have no idea of how homebrew formula files work. Is it possible for the formula to auto-track pacparser releases (for example, I just cut a new release - 1.3.7).
There was a problem hiding this comment.
@manugarg np, I need to finish up this PR with the requested tests and I'll update to 1.3.7 at the same time. We use pacparser in our go applications with my go-pacparser wrapper. Thank you for the time and effort developing the library and test tool.
There was a problem hiding this comment.
@leprechau Sounds good. Thanks!
There was a problem hiding this comment.
Been a bit tied up with other projects the last few days but I will get this updated, hopefully this week. I haven't forgotten about it yet ;p
|
@MikeMcQuaid @manugarg updated to 1.3.7 and replicated the test cases in the test section of the formula |
aaronhurt
changed the title
Adding pacparser (github.com/pacparser/pacparser). A library to parse proxy auto-config (PAC) files.
aaronhurt
force-pushed
the
new/pacparser
branch
from
73703e3 to
496e8b9
Compare
|
squished and pushed as a single commit |
|
Thanks for your contribution to Homebrew! Without people like you submitting PRs we couldn't run this project. You rock! |
Adding pacparser (github.com/pacparser/pacparser). A library to parse proxy auto-config (PAC) files.