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

Dependency upgrades #28

Closed
wants to merge 3 commits into from
Closed

Dependency upgrades #28

wants to merge 3 commits into from

Conversation

bkchr
Copy link
Contributor

@bkchr bkchr commented Feb 26, 2018

Fixes: #27

@bkchr
Copy link
Contributor Author

bkchr commented Feb 26, 2018

I think that somehow broke something. My test machine logs everything with kern_emerg and almost everything in a new line. I will need to further debug this..

@bkchr
Copy link
Contributor Author

bkchr commented Feb 27, 2018

Okay, I could fix the problem. It was not my fault, it was already present. We should create some tests to cover all this.
@Geal are you willing to merge this?

@daboross
Copy link

daboross commented Feb 28, 2018

IIRC format!() is really just write!() to a allocated string which then can be written with. This has a small performance penalty, and also seems a bit like a workaround rather than an actual fix. Do you think it'd be alright to separate that bugfix out into a separate PR for separate review/merging?

Both format!() / .write() and write!() will end up calling the exact same methods with the exact same bytes; the only difference I know of is that format!() allows batching things together. If format!() fixes the bug, it sounds like the actual bug is in LoggerBackend's Write implementation, which doesn't treat bytes equally.

I'm all for upgrading the dependencies, but it would be nice to be able to discuss and fix the bugfix separately from this main change.

@bkchr
Copy link
Contributor Author

bkchr commented Feb 28, 2018

Yeah, I know that format! and write! should go almost the same path. The only difference I can imagine is that write! writes byte by byte or something similar and the syslog implementation reads it byte by byte. On my test machine I saw that each log message got split across several log messages.
I can create a different pull request, just with "the fix"/hack in it. I'm open to a better solution, but currently I don't have any :/

@Geal
Copy link
Owner

Geal commented Feb 28, 2018 via email

@bkchr
Copy link
Contributor Author

bkchr commented Feb 28, 2018

@Geal yeah, I already created a new pull request. And yeah, I run rustfmt, but I created a separate commit for this, so the real changes can be seen easily.

@oherrala
Copy link
Contributor

oherrala commented Apr 5, 2018

This PR also fixes #32.

@bkchr
Copy link
Contributor Author

bkchr commented Apr 5, 2018

@Geal any chance that this could be merged?
I also would be open to help you maintaining this crate.

@Geal
Copy link
Owner

Geal commented Apr 7, 2018

Hello,

I merged all the commits except the rustfmt one. I'll see if I can make a format pass before the release.
@bkchr I'll give you commit rights to the repo, thanks for your help :)

@Geal Geal closed this Apr 7, 2018
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.

None yet

4 participants