fix: Adjust platform detection logic in Stride.Core.targets file

[Jklawreszuk]

PR Details

Temporary fix for Linux. It brings platform detection logic from Stride.Platform.props, so It makes setting StridePlatform msbuild variable fully optional.

Types of changes

• Docs change / refactoring / dependency upgrade
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• My change requires a change to the documentation.
• I have added tests to cover my changes.
• All new and existing tests passed.
• I have built and run the editor to try this change out.

[ds5678]

fix for Linux

What is broken? Is there a GitHub issue about the problem?

Temporary fix

Why temporary?

[ds5678]

Why are you clearing StridePlatform?

[ds5678]

Why did you reverse the priority of RuntimeIdentifier and TargetPlatformIdentifier?

It's seems like consistency with Stride.Platform.props.

stride/sources/sdk/Stride.Build.Sdk/Sdk/Stride.Platform.props

Lines 25 to 36 in 7fadc52

	<StridePlatform></StridePlatform>
	<StridePlatform Condition=" '$(RuntimeIdentifier.StartsWith(win))' ">Windows</StridePlatform>
	<StridePlatform Condition=" '$(RuntimeIdentifier.StartsWith(osx))' ">macOS</StridePlatform>
	<StridePlatform Condition=" '$(RuntimeIdentifier.StartsWith(linux))' ">Linux</StridePlatform>
	<StridePlatform Condition=" '$(StrideTargetFramework)' == '$(StrideFrameworkWindows)' ">Windows</StridePlatform>
	<StridePlatform Condition=" '$(StrideTargetFramework)' == '$(StrideFrameworkmacOS)' ">macOS</StridePlatform>
	<StridePlatform Condition=" '$(StrideTargetFramework)' == '$(StrideFrameworkUWP)' ">UWP</StridePlatform>
	<StridePlatform Condition=" '$(StrideTargetFramework)' == '$(StrideFrameworkAndroid)' ">Android</StridePlatform>
	<StridePlatform Condition=" '$(StrideTargetFramework)' == '$(StrideFrameworkiOS)' ">iOS</StridePlatform>
	<StridePlatform Condition=" '$(StridePlatform)' == '' And '$([MSBuild]::IsOSPlatform(Linux))' ">Linux</StridePlatform>
	<StridePlatform Condition=" '$(StridePlatform)' == '' And '$([MSBuild]::IsOSPlatform(OSX))' ">macOS</StridePlatform>
	<StridePlatform Condition=" '$(StridePlatform)' == '' ">Windows</StridePlatform>

[Jklawreszuk]

Yes, I've tried to replicate logic from the Stride.Platform.props file

[Kryptos-FR]

I'm not sure to understand the fix. .props files are loaded before any .targets file. So if the logic already exists in Stride.Platform.props then there is no need to override it in a .targets file. (edit: it's used for different contexts. Stride.Platform.props is for our stride projects (engine and editor), while Stride.Core.targets is for game projects that are using Stride).

What is exactly the bug that this fixes?

In addition, not checking that the value hasn't already been set doesn't make it optional, it makes it impossible to set in the command line.

edit: ok so after reading the content of Stride.Platform.props, I kind of understand the PR. With that said, I'm not sure either are correct now. In defeat the purpose of being able to set StridePlatform and through that allow cross-compilation.

[ds5678]

I'm not sure to understand the fix. .props files are loaded before any .targets file. So if the logic already exists in Stride.Platform.props then there is no need to override it in a .targets file.

According to a message in Discord, this pull request is based on #3075. However, I didn't know much about .props and .targets files when making that pull request. I just assumed that having logic in all three files was necessary and adjusted the logic to be "complete". Should I have removed the .targets logic?

[Jklawreszuk]

I am sorry my PR description wasn't specific enough, I should have gone into more detail about the situation.
The thing is I found the bug on the latest build (4.4.0-beta4):
As a developer, If I create a new project - either through Stride.Cli or by setting up a new console project and adding packages and I DO NOT manually set the StridePlatform variable to Linux in the .csproj file (e.g., MyGame.Linux), the StridePlatform still automatically defaults to Windows.
Result of binlog on Linux:

This is happening due to the current logic within the Stride.Core.targets file, which is loaded via the Stride packages.

Also, I understand that the @ds5678's fixes were previously located in Stride.Core.props, which I believe was removed in favor of Stride.Platform.props (?), which is part of the currently developing Stride.Build.Sdk. I realize that the SDK is not yet available for use in the projects, which is why I referred to this as a 'temporary' fix."

[ds5678]

I think 3 lines are sufficient to solve your issue: these two and

<StridePlatform Condition="'$(StridePlatform)' == '' And $(RuntimeIdentifier.StartsWith('win'))">Windows</StridePlatform>

[ds5678]

However, I agree that TargetPlatformIdentifier and RuntimeIdentifier should be switched for consistency with Stride.Platform.props.

[Jklawreszuk]

@Kryptos-FR Well, maybe you're right. I will close the PR for now and I’ll try to take a closer look at the new SDK and how to integrate it, as it’s something I’ve completely missed and new to me. Sorry for the confusion 😬️

[ds5678]

I don't think this should be closed.

[Kryptos-FR]

Me neither. It's a good discussion to have. I believe you were on something.

Game projects don't use the SDK, only our internal engine projects. So Stride.Core.targets might need some tweaks. I was confused earlier (I edited my comments since then).

[Jklawreszuk]

Ohhhh I see, I thought the SDK was also designed for the game projects as well.

[ds5678]

I didn't test anything, but I think these changes would resolve your issue:

ds5678@372c3df

[Jklawreszuk]

@ds5678 Yes, your changes are completely correct! I can apply that

[ds5678]

Why not the changes to Stride.Platform.props too?

[Jklawreszuk]

Sure, Done! And most importantly, It works again :

[Kryptos-FR]

If those rules aren't needed anymore, we might need to review Stride.Frameworks.props.

[ds5678]

I took a look at the file you mentioned.

<StrideTargetFramework Condition="$(TargetFramework.StartsWith('$(StrideFrameworkWindows)'))">$(StrideFrameworkWindows)</StrideTargetFramework>

<!-- Written by me for comparision -->
<StrideTargetFramework Condition="'$(TargetPlatformIdentifier)' == 'Windows'">$(StrideFrameworkWindows)</StrideTargetFramework>

These two lines are basically the same since TargetPlatformIdentifier is internally calculated by parsing TargetFramework.

So I agree that if StrideTargetFramework isn't used anywhere else, it could be removed since this code now uses the "official" way of extracting the platform from the TargetFramework.