-
Notifications
You must be signed in to change notification settings - Fork 58
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
Readme updates #137
Readme updates #137
Conversation
Update README.md to account for recent llhttp build changes and the availability of packaged versions for FreeBSD.
Missed one llhttp
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #137 +/- ##
=======================================
Coverage 57.60% 57.60%
=======================================
Files 4 4
Lines 552 552
Branches 145 145
=======================================
Hits 318 318
Misses 133 133
Partials 101 101 ☔ View full report in Codecov by Sentry. |
A few more updates to reflect changes in more recent versions.
Fix cut and paste error
My original thinking had been to update the README to show llhttp as the required library. But, of course, the Debian-based distros don't have it and it looks like CentOS 7 and CentOS Stream 8 don't either. CentOS Stream 9 is being built with http-parser, although llhttp is available in EPEL 9. So, now I'm thinking maybe the README should be adjusted to be clearer that it works with both. I was just trying to make it reflect recent events; I don't have a preference if someone else changes it. I'm certainly also happy to make the adjustments to the PR if that's what's preferred. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your change. In general, it LGTM. Requesting some minimum changes to distinguish between distributions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your changes. As you commented, please, change to include both dependencies according to the kind of distribution. Something like this would be enough:
Tang requires a few other software libraries:
- CentOS / Debian based distributions:
- https-parser >= 2.8.0
- systemd
- jose
- Rest of distributions:
- llhttp - ...
- systemd
- jose
Noted that either http_parser or llhttp reequired. Removed systemd from the required list, since strictly speaking it's not required for stand-alone operation.
Changed to note that either http_parser or llhttp reequired. Removed systemd from the required list, since strictly speaking it's not required for stand-alone operation. |
Clean up the markup that went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM
Update README.md to account for recent llhttp build changes and the availability of packaged versions for FreeBSD.