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

Handle a dot character in project name correctly #960

Open amz-shahji opened 5 years ago

amz-shahji commented 5 years ago

A dot is not interpreted corrected in project names. For a project name like 'abc.def', the generated dll is 'abc.dll' instead of 'abc.def.dll'. Affects the output of both static and shared libraries. Haven't been confirmed for applications.

fletcher-sumglobal commented 5 years ago

When the project name contains '.', the code assumes it is a delimeter for a file extension. The code that does this is in org.gradle.internal.FileUtils.java in subprojects/base-services. Not sure what sorts of projects depend on this removal of extensions and if a PR on this would fly. IMHO the code shouldn't assume you have an extension after the '.'

Here is the line of code -> https://github.com/gradle/gradle/blob/bfd82e349e695b945c7d8de864cc2db9a182d65f/subprojects/base-services/src/main/java/org/gradle/internal/FileUtils.java#L179

big-guy commented 5 years ago

I don't think we intentionally want the current behavior.

I think the fix for this is to not use withExtension at all. It looks like we're getting a partial path thing, turning it into another partial path, working backwards to remove one extension and then adding another extension.

e.g., https://github.com/gradle/gradle/blob/622ba948618e16124d0ab3d09699d5c105d1e4de/subprojects/platform-native/src/main/java/org/gradle/nativeplatform/toolchain/internal/UnavailablePlatformToolProvider.java#L110

I think this is what happens today:

This could be much simpler. I reckon something like:

There are several variants of getLibrarySymbolFileName that are doing something similar for static libraries, executables, etc. All of these would need to be tweaked in this way.

Does that make sense?

fletcher-sumglobal commented 5 years ago

I think you are saying these methods 'getSharedLibraryName(String libraryPath)' , 'getExecutableName(String executablePath)' and others like it need to have 'withExtension' method removed. These methods are all in 'org.gradle.internal.os.OperatingSystem', and are what is getting used at execution time for the VisualCppPlatformToolProvider on windows, and not the ones in 'org.gradle.nativeplatform.toolchain.internal.UnavailablePlatformToolProvider'. I have found the only scenario that uses the 'withExtension' method is for Windows/Vs variant. Other tool chains, and OS's aren't affected by the 'withExtension' method to my knowledge.

This easiest solution is to remove calls to 'withExtension' altogether coming form the OperatingSystem class, and just return what is passed into it, /libname + ".some-extension", because that is essentially what 'withExtension' did previously, only it returned a different result if you had a '.' in the path passed in. I can put together a PR on this, and you guys can either take it as is, or massage it into a more strongly modeled approach. If I change the abstract method signatures on 'OperatingSystem', this results in a much bigger change having to have the signatures changed in every method upstream that calls it, so to minimize that, I can just drop the 'withExtension' method which really only impact the Windows/Vs variant.

LouisCAD commented 1 year ago

https://github.com/gradle/gradle/commit/b64d51b4fe6797b2ea20db75b9c20503bf2e4139 wasn't merged, after all?