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

windows compatibility #35

Merged
merged 3 commits into from
Jun 9, 2017
Merged

windows compatibility #35

merged 3 commits into from
Jun 9, 2017

Conversation

sebasv
Copy link
Contributor

@sebasv sebasv commented Jun 7, 2017

On windows machines, the file mode is less relevant than on unix machines. On top of that the std::os::unix::fs::PermissionsExt crate has no windows alternative as of this moment, so the highlighting of executable files is disabled with a conditional compilation directive that verifies the target os.

src/main.rs Outdated
@@ -12,6 +12,7 @@ use std::error::Error;
use std::ffi::OsStr;
use std::fs;
use std::io::Write;
#[cfg(target_os = "linux")]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this still work for other Unix-like operating systems (OSX, ..)?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't look like it; this should probably be something like target_family = "unix".

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, seems like we can use

#[cfg(unix)]

and

#[cfg(not(unix))]

@sharkdp
Copy link
Owner

sharkdp commented Jun 7, 2017

Thank you very much!

I'm fine with the dummy-behavior for isExecutable on windows.

@sebasv sebasv mentioned this pull request Jun 7, 2017
@Boscop
Copy link

Boscop commented Jun 8, 2017

Maybe is_executable() on windows should return true if the extension is .exe or .bat?

@BatmanAoD
Copy link

@Boscop As a point of comparison, it appears that Git-Bash treats .exe and .dll files as executable, but not .bat files. But a Windows system on which .bat files aren't executable is essentially broken.

To my mind, special-case logic trying to guess how file extensions should be treated is almost certainly the wrong approach. It is a can of worms that gets increasingly complicated; for instance, script-language files (.py, .pl, .rb, etc) may or may not be executable depending on how file extension handling is set up, which depends on registry settings. My opinion (for what it's worth) is that it's better to be simple and consistent.

@sebasv
Copy link
Contributor Author

sebasv commented Jun 9, 2017

I second @BatmanAoD , executability is determined by file-wise global settings on windows but they are user-specific. Trying to infer executability based on extension is confusing, and I don't think there currently is a clear and simple method to extract this information from the registry.

@sharkdp
Copy link
Owner

sharkdp commented Jun 9, 2017

In the end, is_executable only decides about the output color, so it's probably not too bad if there are some executable files that are not highlighted.

I'm okay with either of:

  • always return false => nothing get's highlighted
  • check the extension for exe and bat and only highlight those

@sebasv What do you think about the #[cfg(unix)] change? Do you want to change that?

change target_os->target_family
@sebasv
Copy link
Contributor Author

sebasv commented Jun 9, 2017

@sharkdp #[cfg(unix)] is indeed more appropriate since the crate in question seems to target all unix systems. I initially chose for #[cfg(target_os="linux")] out of ignorance, I didn't know of the target_family option until now!

src/main.rs Outdated
@@ -77,13 +78,17 @@ fn print_entry(base: &Path, entry: &Path, config: &FdOptions) {
None => return
};

#[cfg(target_os = "unix")]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be target_family here, too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, well spotted. I appreciate the keen eye!

src/main.rs Outdated
let is_executable = |p: &std::path::PathBuf| {
p.metadata()
.ok()
.map(|f| f.permissions().mode() & 0o111 != 0)
.unwrap_or(false)
};

#[cfg(not(target_os = "unix"))]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be target_family here, too

@sharkdp sharkdp merged commit f7910c1 into sharkdp:master Jun 9, 2017
@sharkdp
Copy link
Owner

sharkdp commented Jun 9, 2017

Thank you very much!

@BatmanAoD
Copy link

Awesome! Can't wait to try this out now!

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