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

Reduce redundant properties related to the platform or target #963

Closed
big-guy opened this issue Dec 7, 2018 · 10 comments
Closed

Reduce redundant properties related to the platform or target #963

big-guy opened this issue Dec 7, 2018 · 10 comments
Assignees

Comments

@big-guy
Copy link
Member

big-guy commented Dec 7, 2018

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
Copy link
Member

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
Copy link
Member Author

big-guy commented Dec 10, 2018

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
Copy link
Member Author

big-guy commented Dec 10, 2018

related #399

@lacasseio lacasseio self-assigned this Jan 10, 2019
@lacasseio
Copy link
Contributor

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 gradle/gradle@d2018d9 for more information about the work in progress.

@lacasseio
Copy link
Contributor

I created a PR regarding the above: gradle/gradle#8222

@lacasseio
Copy link
Contributor

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
Copy link
Member Author

big-guy commented Jan 14, 2019

@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:

  • a target machine refers to the operating system and architecture of a binary. It's language-agnostic and somewhat similar to a target triplet.
  • a target platform refers to the runtime environment of a binary, which includes the target machine.

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:

  • Remove targetPlatform (like you have) from ComponentWithNativeRuntime and replace it with a TargetMachine targetMachine.
  • Keep the name CppPlatform and SwiftPlatform types (vs xxxTargetMachine). Keep the targetPlatform variable name.
  • CppPlatform and SwiftPlatform should not extend TargetMachine, but have a targetMachine property.

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
Copy link
Contributor

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

@lacasseio
Copy link
Contributor

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

@lacasseio
Copy link
Contributor

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

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

No branches or pull requests

3 participants