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

Add property UsingGodotNETSdk to Godot.NET.Sdk #89652

Merged
1 commit merged into from
Mar 24, 2024
Merged

Add property UsingGodotNETSdk to Godot.NET.Sdk #89652

1 commit merged into from
Mar 24, 2024

Conversation

invino4
Copy link
Contributor

@invino4 invino4 commented Mar 18, 2024

The Godot.NET.Sdk (for C# use) should define the property UsingGodotNETSdk in its SDK.props.

Why

Defining this property allows shared build configuration (e.g. Directory.Build.targets or other imported msbuild files) to detect deterministically when they are operating within the scope of a project controlled by Godot.NET.Sdk. This enables shared build configuration that may span many different projects within a folder to have Godot.NET.Sdk-specific configuration that only applies to Godot projects.

Why named UsingGodotNETSdk

This naming scheme is common practice in Microsoft Dotnet SDKs. For example, the property UsingMicrosoftNETSdk is defined by the default SDK, UsingMicrosoftTraversalSdk is defined by Microsoft.Build.Traversal, UsingMicrosoftNoTargetsSdk by Microsoft.Build.NoTargets, and so on. The property UsingMicrosoftNETSdk is even used in the implementation of Godot.NET.Sdk for conditional logic of the type predicted here.

Note that these "Using*" properties are additive in the sense that more than one can be defined for a given project (as SDKs can effectively be built upon other SDKs, using them as components). So, it is normal and appropriate for both UsingMicrosoftNETSdk and UsingGodotNETSdk to be simultaneously defined within the same project.

The Godot.NET.Sdk (for C# use) should define the property `UsingGodotNETSdk` in its [SDK.props](https://github.com/godotengine/godot/blob/a07dd0d6a520723c4838fb4b65461a16b7a50f90/modules/mono/editor/Godot.NET.Sdk/Godot.NET.Sdk/Sdk/Sdk.props).  

## Why
Defining this property allows shared build configuration (e.g. Directory.Build.targets or other imported msbuild files) to detect deterministically when they are operating within the scope of a project controlled by Godot.NET.Sdk.  This enables shared build configuration that may span many different projects within a folder to have Godot.NET.Sdk-specific configuration that only applies to Godot projects.

## Why named UsingGodotNETSdk
This naming scheme is common practice in Microsoft Dotnet SDKs.  For example, the property `UsingMicrosoftNETSdk` is defined by the default SDK, `UsingMicrosoftTraversalSdk` is defined by [Microsoft.Build.Traversal](https://github.com/microsoft/MSBuildSdks/blob/363532de5b406c9afc6e6ff0f276431c27b11347/src/Traversal/Sdk/Sdk.props#L10), `UsingMicrosoftNoTargetsSdk` by [Microsoft.Build.NoTargets](https://github.com/microsoft/MSBuildSdks/blob/363532de5b406c9afc6e6ff0f276431c27b11347/src/NoTargets/Sdk/Sdk.props#L10), and so on.  The property `UsingMicrosoftNETSdk` is even used in the implementation of Godot.NET.Sdk for conditional logic of the type predicted here.

Note that these "Using*" properties are _additive_ in the sense that more than one can be defined for a given project (as SDKs can effectively be built upon other SDKs, using them as components).  So, it is normal and appropriate for both `UsingMicrosoftNETSdk` and `UsingGodotNETSdk` to be simultaneously defined within the same project.
@invino4 invino4 requested a review from a team as a code owner March 18, 2024 16:50
@AThousandShips AThousandShips added this to the 4.x milestone Mar 18, 2024
@paulloz
Copy link
Member

paulloz commented Mar 22, 2024

Hello, thanks for the PR 🙂
I'm not especially against this, but I am wondering if the use-case isn't already covered by the SDK defining GodotDefineConstants here?

@invino4
Copy link
Contributor Author

invino4 commented Mar 22, 2024

Hey Paul, I had similar thoughts about just using GodotDefineConstants but I decided I didn't really like it. My reasoning stems from one of a set of design principles I usually try to follow (4: Code Not Convention) which argues "If it matters, then make it explicit". Being able to distinguish in a shared context (such as Directory.Build.targets) whether you are operating within a Godot project matters for both the correctness and the determinism of the build.

While it might be possible to distinguish using some coincidental property, such as GodotDefineConstants, this is not the explicit purpose of GodotDefineConstants. Assuming additional semantics for GodotDefineConstants has a code smell since it violates the abstraction boundary of Godot.NET.Sdk. GodotDefineConstants isn't meant to be part of the external interface of Godot.NET.Sdk, and so using it directly is like accessing a private variable in a class. It's an implementation detail of the SDK. If a future SDK author decides to come along and rename, split, make conditional, or otherwise change the nature of GodotDefineConstants they would likely not think at all about the consequences to outside projects that have taken an external dependency on these additional semantics. Instead, those projects would be broken by the change. This makes an assumption brittle.

Defining UsingGodotNETSdk, however, has the opposite impact. It makes a deliberate external specification and defines an interface with external contractual guarantees. It will never be a property that is used for some other purpose and would never be subject to unrelated refactoring. Lastly, since its defined semantics are an external contract, refactoring UsingGodotNETSdk cannot be done without considering the impact to dependent projects (because doing so is, by definition now, a breaking change).

Plus, this PR is trivial. :)

As a historical note, the Dotnet SDK didn't originally define UsingMicrosoftNETSdk and coincidental properties within the SDK were often used as a workaround. Over time, evolution of the SDK demonstrated the brittleness of this usage, and projects were broken. UsingMicrosoftNETSdk was introduced to solve that real-world problem.

@paulloz
Copy link
Member

paulloz commented Mar 23, 2024

Thanks for the detailed write up. Yes. I agree with explicitness being a good thing, and the fact the constants might evolve with time, etc.
Not sure how other people see this, but this looks good to me.

Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me, it's a small change and seems consistent with other SDKs.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Mar 23, 2024
@akien-mga akien-mga changed the title Add property UsingGodotNETSdk to Godot.NET.Sdk Add property UsingGodotNETSdk to Godot.NET.Sdk Mar 24, 2024
@akien-mga akien-mga closed this pull request by merging all changes into godotengine:master in 06abc86 Mar 24, 2024
akien-mga added a commit that referenced this pull request Mar 24, 2024
Add property `UsingGodotNETSdk` to Godot.NET.Sdk
@akien-mga
Copy link
Member

Thanks!

@invino4 invino4 deleted the patch-1 branch March 25, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants