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.

Sign up for GitHub

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

Jump to bottom

Remove wrk from tree #982

Closed
wants to merge 2 commits into from
Closed

Remove wrk from tree #982

wants to merge 2 commits into from

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.

@jbergstroem
build: remove tools/wrk from the tree
3bc1fb0 
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%
@jbergstroem jbergstroem mentioned this pull request Feb 27, 2015
Closed
@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!

Fishrock123
Fishrock123 reviewed Feb 27, 2015
View reviewed changes
benchmark/README.md
```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
@jbergstroem @rvagg
build: remove tools/wrk from the tree
a45d4f8 
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
@jbergstroem @rvagg
benchmark: check for wrk ahead of running benchmarks
2b79052 
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

@rvagg rvagg closed this Mar 5, 2015
@rvagg rvagg mentioned this pull request Mar 5, 2015
Closed
This was referenced Jul 8, 2023
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jbergstroem @Fishrock123 @rvagg @mscdex