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

Reduce redundant properties related to the platform or target #963

Closed big-guy closed 5 years ago

big-guy commented 5 years ago

We don't currently expose the TargetMachine used by the binary, but we do have a "target platform" (using software model types) that are extended by the C++ plugin as CppPlatform. This is somewhat the same thing as TargetMachine.

I think we want to expose TargetMachine in ComponentWithNativeRuntime and hide NativePlatform (we'll need it to work with the existing tasks). We may be able to get rid of CppPlatform entirely.

We should consider how this affects our samples that demonstrate target machine specific dependencies.

adammurdoch commented 5 years ago

We should certainly replace NativePlatform on the new types.

If we get rid of CppPlatform we should also get rid of SwiftPlatform. Just imagine there are some properties that describe the C++ language version or dialect or the C++ standard library on CppPlatform.

big-guy commented 5 years ago

from planning... The TargetMachine on the binary may have extra details or more information than the TargetMachine specified on the component.

👍 CppPlatform/SwiftPlatform should represent the language details/target.

big-guy commented 5 years ago

related #399

lacasseio commented 5 years ago

What I set off implementing for this issue is to break the polymorphism between CppPlatform/SwiftPlatform and NativePlatform to instead have CppTargetMachine/SwiftTargetMachine extends TargetMachine. The binaries now return a TargetMachine type that will include the SwiftVersion and later the C++ source compatibility in the future. I also tried to clean up the TargetMachine interface to only have getters on it. See https://github.com/gradle/gradle/commit/d2018d975f66fdb00e1563570cb2208d974784ff for more information about the work in progress.

lacasseio commented 5 years ago

I created a PR regarding the above: https://github.com/gradle/gradle/pull/8222

lacasseio commented 5 years ago

The only other property that is shared with the software model is the ComponentWithNativeRuntime#getToolChain(). I don't think we want to remove that property just yet.

big-guy commented 5 years ago

@lacasseio I took a look at the PR and I think you're close to what I was imagining.

I think we should go with something like:

My expectation is that you could have many target platforms that have similar target machines (e.g., only differ with SDK/language levels).

I'd suggest a couple of tweaks:

This makes ComponentWithNativeRuntime somewhat language agnostic and you can use binary.targetMachine as you like. But for language specific platform things (e.g., source compatibility), you can access that via binary.targetPlatform.sourceCompatibility and you can get to architecture/OS as well.

WDYT?

lacasseio commented 5 years ago

I think I got it, let me do some change to the PR.

lacasseio commented 5 years ago

@big-guy I pushed some changes to reflect your comments. What do you think about the changes?

lacasseio commented 5 years ago

I find binary.targetPlatfrom.targetMachine and binary.targetMachine duplication a bit strange. I'm not too sure what should be the solution though.