WIP: Never pick up Strawberry Perl's pkg-config as a valid implementation

[nirbheek]

Strawberry Perl places a pkg-config.bat into PATH that is written in Perl and is not intended to be used by third parties as a MinGW distribution. This wouldn't matter, except that Strawberry Perl is also included in Github CI images out of the box, in PATH, and it breaks everyone's CI jobs.

This is already done by Meson and CMake:

https://github.com/mesonbuild/meson/pull/9384

https://gitlab.kitware.com/cmake/cmake/-/merge_requests/9375

Fixes https://github.com/rust-lang/pkg-config-rs/issues/164

This could've used the which crate, but we don't want dependent crates, so we do a manual search instead.

WIP because I haven't tested this yet.

[sdroege]

Generally makes sense to me. Once this is confirmed to work and merged, I'll get a new release out.

[nirbheek]

I had to rework the code a bit, it had the following mistakes:

1. PATHEXT is .COM;.EXE;.BAT etc, so we need to strip the starting . otherwise PathBuf.set_extension() will not do the right thing
2. Need to also look in the current directory for pkg-config, which is what CreateProcess also does.

I also added some CI and now it looks like 1.30.0 doesn't contain eq_ignore_ascii_case(). OTOH, I looked at how it's implemented and I don't think it does what we want it to do... Need some testing.

I'll write a test for ignoring Strawberry Perl's pkg-config.

[ChrisDenton]

Need to also look in the current directory for pkg-config, which is what CreateProcess also does.

I've not been following this PR so I don't have much context but do note that rustc considers this a security issue and deliberately avoids searching for executable things in the current directory. E.g. ripgrep had a CVE filed against it for this behaviour (see https://github.com/BurntSushi/ripgrep/commit/229d1a8d41b0023420e7815578fa0b39c0d5c2e4).

[nirbheek]

do note that rustc considers this a security issue and deliberately avoids searching for executable things in the current directory

I don't know how relevant that is for a build system, considering that when you execute cargo build you are literally executing arbitrary commands from the current directory, especially due to build.rs :wink:

However, while investigating all this, I found something very hilarious. Strawberry Perl's pkg-config is already not found by pkg-config-rs because Rust's std::process::Command can only find .exe files unless you set an env var, in which case it can find other executables, but it will never pick extensions from PATHEXT, so it will never find pkg-config.bat when looking for pkg-config: https://github.com/rust-lang/rust/issues/37519.

It's all very broken, but also this PR is not needed...

[ChrisDenton]

The exact rules can (and very likely will) change. Basically, in the pre-1.0 days it attempted to follow Linux rules. Except over time this became broken and fell back to CreateProcess defaults... except when it didn't. Most of the actual breakage in that issue has been fixed but the difference between using a default environment and setting environment variables was deliberately kept while it's decided what the rules should be (i.e. follow Linux rules, CreateProcess rules or some hybrid),

[nirbheek]

Thanks for the explanation! I expect that this will change some day, so we should keep this PR around, but I don't know if we should merge code that actually does nothing today. Especially since this PR's purpose is to try to match what Command does, and that is expected to change over time.

What I do think we should do is to merge a test that checks that Strawberry Perl pkg-config is not picked up. Anything beyond that would just be added maintenance burden.

Honestly, I'm surprised that Rust's std decided to not learn from what other APIs are doing in this problem space, dooming itself to repeat everyone else's mistakes. We haven't even gotten to arguments yet — the API takes a list, but CreateProcess takes a single string.