search

rust-lang / jobserver-rs

Apache License 2.0
69 stars 39 forks source link

Diagnostic: extend from_env function #52

Closed belovdv closed 1 year ago

belovdv commented 1 year ago

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.

belovdv commented 1 year ago

Thanks for review, I'll wait other's comments.

belovdv commented 1 year ago

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.

alexcrichton commented 1 year ago

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 fcntl not sufficient and what is fstat catching otherwise? Is that catching real issues happening in the wild and allowing callers to recover?

petrochenkov commented 1 year ago

I don't think that the Cargo feature added here will be all that useful

+1

Can you explain a bit more what the error handling around pipe detection is doing? Why is fcntl not sufficient and what is fstat catching otherwise?

The pipe check is the same thing that jobserver-rs did before https://github.com/alexcrichton/jobserver-rs/pull/6.

Is that catching real issues happening in the wild and allowing callers to recover?

It catches a real issue that happens in the wild. The caller won't be able to recover, but it will be able to report a useful error that will allow the "second order caller" (build engineer) to fix the build environment.

alexcrichton commented 1 year ago

If this is reverting back, is the use case in https://github.com/alexcrichton/jobserver-rs/pull/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".

petrochenkov commented 1 year ago

I meant moreso the new logic to detect a valid file descriptor

Yes, I meant the corrupted non-pipe descriptors appear in the wild.

If this is reverting back, is the use case in https://github.com/alexcrichton/jobserver-rs/pull/6 no longer valid?

The PR author says that the use case is no longer relevant in https://github.com/alexcrichton/jobserver-rs/issues/27#issuecomment-683162244. Also, descriptor being a pipe seems to be a POSIX requirement - https://github.com/alexcrichton/jobserver-rs/pull/52#discussion_r1135263485.