Modify executable checking to be more universal#76607
Conversation
rust-highfive
added
the
S-waiting-on-review
|
cc @mati865 I don't have a Windows/WSL system to test this on. |
|
It felt like working on Core 2 Duo system again but |
|
Interesting! It looks like the /checkout/src directory is actually read-only in our CI environment, so this check would then be a bit problematic. @pietroalbini, do you know if we mount that directory as read-only for any particular reason? Could we make that not true? I guess the alternative is to try and disable the touching just in CI... |
|
I don't know exactly why (I wasn't the one doing that, and the comments don't explain it), but I guess it's to force every tool and proc macro not to override the source code, but use Cargo's |
|
Hm. In this case we're intentionally trying to detect src because other directories might, in theory be on different partitions... I guess we can fall back to trying the same pattern in the build directory. It'll almost always be on the same filesystem. |
jyn514
added
A-testsuite
This uses a dummy file to check if the filesystem being used supports the executable bit in general.
Mark-Simulacrum
force-pushed
the
tidy-bins
branch
from
e687789 to
05c9c0e
Compare
|
Okay, I think this should be good on our CI now -- mingw-check no longer skips the bins check, at least. |
crlf0710
added
S-waiting-on-review
| if contents.contains("Microsoft") || contents.contains("boot2docker") { | ||
| return; | ||
| fn is_executable(path: &Path) -> std::io::Result<bool> { | ||
| Ok(path.metadata()?.mode() & 0o111 != 0) |
There was a problem hiding this comment.
So my only misgiving here is that when I first read this, I spent a while wondering about why its looking at the group and others bits at all, and about what scenarios could arise here. (example questions: Does owner alone not suffice? Is there any scenario where we could get an exec-bit set for group and/or others, and not owner, and yet we would still expect things to continue to work? Because the logic as written here seems to imply that interpretation of matters...)
I don't really know the best way to fix this, or if it really is something to fix at all. I figure the code as written is probably just playing it safe (no one ever got fired for taking the OR of all three triads, right?). But I would be curious if there is actually a scenario that its anticipating, and if so, maybe a comment here is warranted.
There was a problem hiding this comment.
Actually, I guess given the first context where this is called, namely a check if the filesystem is magically treating fresh created files as executable, the OR of all the triads does make sense to me.
Update: And for the second context, where we are trying to detect a binary that has been checked into git... well, I guess that was the logic that was already there, so we might assume that's doing the right thing.
(I might rename the method to is_any_triad_executable, though, to make that clear.)
|
@bors r+ rollup |
|
📌 Commit 05c9c0e has been approved by |
bors
added
S-waiting-on-bors
Rollup of 7 pull requests Successful merges: - rust-lang#75802 (resolve: Do not put nonexistent crate `meta` into prelude) - rust-lang#76607 (Modify executable checking to be more universal) - rust-lang#77851 (BTreeMap: refactor Entry out of map.rs into its own file) - rust-lang#78043 (Fix grammar in note for orphan-rule error [E0210]) - rust-lang#78048 (Suggest correct place to add `self` parameter when inside closure) - rust-lang#78050 (Small CSS cleanup) - rust-lang#78059 (Set `MDBOOK_OUTPUT__HTML__INPUT_404` on linkchecker) Failed merges: r? `@ghost`
9 participants
This uses a dummy file to check if the filesystem being used supports the executable bit in general.
Supersedes #74753.