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

Remove wrk from tree #982

Closed
wants to merge 2 commits into from
Closed

Conversation

jbergstroem
Copy link
Member

This removes wrk from the source tree and replaces all calls to it with a check if wrk is installed or not, calling on users/developers to install it otherwise.

wrk is an optional tool that some of the http benchmarks uses. The removal
doesn't affect any users. Developers are assumed to install it before running
the tests.

This change reduces the tarball by 5%
@Fishrock123
Copy link
Contributor

Is size alone enough of a motivation for this? I mean, it's not like the benchmarks are run that often, but..

@jbergstroem
Copy link
Member Author

I see it similar to how you'd compare using the result of make dist in autotools versus a git checkout of the same tree. Users and developers have different usage patterns and we haven't really made a distinction between them. I guess I'm treading on that line. Today it's about making a tarball smaller, but tomorrow it could mean less build-time or runtime dependencies.

Edit: expanding on make dist: Autotools will generate a tarball for you that includes generated versions of auto* and other things you might have excluded/added. This makes the number of dependencies required for user much smaller.

@rvagg
Copy link
Member

rvagg commented Feb 27, 2015

LGTM, I'm going to merge this today unless someone objects before I do!

```sh
make wrk
```
Most of the http benchmarks require `wrk` and `ab` being installed.
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs links to those projects, at very least.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to project links

@jbergstroem
Copy link
Member Author

Thanks for the feedback. I'll update shortly.

@rvagg
Copy link
Member

rvagg commented Feb 28, 2015

deferring merge for now, would appreciate feedback from others though, I'm still happy to remove this, it's out of place in the source tree

@jbergstroem
Copy link
Member Author

Not sure who to ping here, but here it is.

rvagg pushed a commit that referenced this pull request Mar 5, 2015
wrk is an optional tool that some of the http benchmarks uses. The removal
doesn't affect any users. Developers are assumed to install it before running
the tests.

This change reduces the tarball by 5%

PR-URL: #982
Reviewed-By: Rod Vagg <rod@vagg.org>
rvagg pushed a commit that referenced this pull request Mar 5, 2015
PR-URL: #982
Reviewed-By: Rod Vagg <rod@vagg.org>
@rvagg
Copy link
Member

rvagg commented Mar 5, 2015

landed in 2b79052 & a45d4f8

thanks @jbergstroem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants