-
Notifications
You must be signed in to change notification settings - Fork 39
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
Diagnostic: extend from_env function #52
Diagnostic: extend from_env function #52
Conversation
0accda8
to
3275a9a
Compare
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.
This looks good to me in general. Let's see other's thought on the change :)
Thanks for review, I'll wait other's comments. |
Cargo.toml
Outdated
@@ -15,7 +15,7 @@ edition = "2018" | |||
libc = "0.2.50" | |||
|
|||
[features] | |||
check_pipe = [] | |||
do_not_check_pipe = [] |
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.
This is also not a good idea since features should be additive.
With this do_not_check_pipe feature,if one of the crate enables this, then every other crate is opt out of checking.
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.
Otherwise if one enables check then one uses smth different from pipe will fail. I'm not sure should be this option or not and, if should, how should it behave, imho it should be decided by maintainer
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.
If checking or not checking the pipe-ness should be configurable, then it should probably be a runtime option passed to from_env
.
Not checking is the GNU Make behavior, checking catches errors in practice.
Thanks for review. Hopefully, I managed to implement most requests and suggestions. As for choice between checking pipe and checking only fd, I'd rather wait for alexcrichton decision. |
Given how low-level this crate is I don't think that the Cargo feature added here will be all that useful since if it's in use it's buried so deep and will be difficult to configure. Can you explain a bit more what the error handling around pipe detection is doing? Why is |
+1
The pipe check is the same thing that jobserver-rs did before #6.
It catches a real issue that happens in the wild. |
If this is reverting back, is the use case in #6 no longer valid? For "catching things in the wild" I meant moreso the new logic to detect a valid file descriptor, less so "should this return errors at all" because that's naturally a "sure this should happen when the effort is put in". |
Yes, I meant the corrupted non-pipe descriptors appear in the wild.
The PR author says that the use case is no longer relevant in #27 (comment). |
72d8e4c
to
c78d26d
Compare
/// Cannot connect following this process's environment. | ||
PlatformSpecific { | ||
/// Error. | ||
err: io::Error, |
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.
@weihanglo
Could you try this interface in cargo before it's in a "stable" release of jobserver-rs?
It's possible that in practice we'll need to discern between different kinds of erros (e.g. "descriptors are closed" and everything else) to avoid bothering users with warnings too often.
This PR, however, merges all the errors into a single io::Error
with multiple vague ErrorKind
s.
I'm not sure users will be able to use it reliably without relying on "string typing" (inspecting string descriptions returned by errors) or implementation details ("we know that closed descriptors are ErrorKind::InvalidData
by reading source code of jobserver-rs").
(I don't like this part and I wish I read this PR in detail before its merge.)
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.
Sure. I'll try taking a look this week.
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.
implementation details ("we know that closed descriptors are ErrorKind::InvalidData by reading source code of jobserver-rs").
Maybe jobserver can have the error kind documented on docs.rs?
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.
Sorry for the late response. I think the change works fine with Cargo. Cargo can just ignore the jobserver if there are bad pipes. No harm to get this patch released on Cargo side I believe :)
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.
RQ
Implements major part of #51.
This pr adds error type for
from_env()
and returns possibility to check if fds refer to pipe as optional feature.