gradle / gradle-native

The home of Gradle's support for natively compiled languages
https://blog.gradle.org/introducing-the-new-cpp-plugins
Apache License 2.0
92 stars 8 forks source link

Header discovery should be resilient to bad candidate filesystem paths #950

Closed ghale closed 5 years ago

ghale commented 5 years ago

Expected Behavior

Header discovery should be able to find header files wherever they exist in the include path, regardless of what other files may be there.

Current Behavior

Let's say we have /some/path/include and /some/path/include/foo on the include path. Then a source file has an include for foo/bar.h. In most cases, this works fine - the header discovery may look for /some/path/include/foo/foo/bar.h, not find it, and continue searching, eventually finding it in /some/path/include/foo/bar.h. However, if there actually is a file at /some/path/include/foo/foo, when we search for /some/path/include/foo/foo/bar.h we get an unexpected error code from the system call to stat(). Instead of errno 2 (no such file), we'll get an errno 20 (not a directory) from the stat system call. This is unexpected and causes an exception, failing the build.

ghale commented 5 years ago

@adammurdoch I created a PR for this here: https://github.com/adammurdoch/native-platform/pull/30

big-guy commented 5 years ago

@ghale is it worth having a integ test on the Gradle side that demonstrates the failure?

ghale commented 5 years ago

Yeah, I actually meant to do that, but in the noise of implementing the fix in native-platform, I forgot. Thanks for reminding me.

ghale commented 5 years ago

I added an integration test for this with https://github.com/gradle/gradle/commit/c4e549662f0c302ed46f31087288119eed0906d2. Once we have a new version of native-platform, we should be able to un-ignore that test and see it pass.

adammurdoch commented 5 years ago

I've created a new release of native-platform. I'm just running the upgrade through CI and will merge if all is good.

adammurdoch commented 5 years ago

Upgraded to new version: https://github.com/gradle/gradle/pull/8146

ghale commented 5 years ago

Thanks @adammurdoch! I've un-ignored the test for this case, too.