-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Fix templates for Linux builds #17205
Conversation
CI error on Windows
|
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
See #16143 I attempted that before, we're not doing that at the time being without some other things being fixed first. |
From Seems like
is not supported. But maybe
could help, for now? Shouldn't that be OK on Linux? Maybe @grendello could give us few hints. Adding that to installation/setup would make more people happy. Not needed already there: https://learn.microsoft.com/en-us/dotnet/maui/get-started/installation?tabs=visual-studio-code#linux |
@moljac yes, |
@@ -1,8 +1,9 @@ | |||
<Project Sdk="Microsoft.NET.Sdk"> | |||
|
|||
<PropertyGroup> | |||
<TargetFrameworks>DOTNET_TFM-android;DOTNET_TFM-ios;DOTNET_TFM-maccatalyst</TargetFrameworks> | |||
<TargetFrameworks Condition="$([MSBuild]::IsOSPlatform('windows'))">$(TargetFrameworks);DOTNET_TFM-windows10.0.19041.0</TargetFrameworks> | |||
<TargetFrameworks>DOTNET_TFM-android</TargetFrameworks> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to fix the indentation on these.
<TargetFrameworks>DOTNET_TFM-android</TargetFrameworks> | ||
<TargetFrameworks Condition=" ! $([MSBuild]::IsOSPlatform('linux'))">$(TargetFrameworks);DOTNET_TFM-ios;DOTNET_TFM-maccatalyst</TargetFrameworks> | ||
<TargetFrameworks Condition="$([MSBuild]::IsOSPlatform('windows'))">$(TargetFrameworks);DOTNET_TFM-windows10.0.19041.0</TargetFrameworks> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that this block of conditions is getting incredibly hard to understand. There's an always-on Android. There's a negative condition ios/maccat, and a positive-condition Windows.
Is there a cleaner way to slice these up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, see #16143
We need to fix the SDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'm wary of making these changes to the template to work around the correct fix to the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a cleaner way to slice these up?
Depends on definition of "cleaner".
If it means "easier to understand" then maybe:
<TargetFrameworks Condition=" $([MSBuild]::IsOSPlatform('windows')) || $([MSBuild]::IsOSPlatform('osx'))">$(TargetFrameworks);DOTNET_TFM-ios;DOTNET_TFM-maccatalyst</TargetFrameworks>
I went for less-cycles concept driven by "green", "sustainable", "shorter inner loop" (for large numbers especially).
Note "windows" as 1st in most cases 2nd condition should not be evaluated (only for mac).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, see #16143
Matthew. I really did not see that PR, but it seems similar, moreover the same if whitespace is ignored.
<TargetFrameworks Condition="!$([MSBuild]::IsOSPlatform('linux'))">$(TargetFrameworks);DOTNET_TFM-ios;DOTNET_TFM-maccatalyst</TargetFrameworks> |
<TargetFrameworks Condition="!$([MSBuild]::IsOSPlatform('linux'))">$(TargetFrameworks);DOTNET_TFM-ios;DOTNET_TFM-maccatalyst</TargetFrameworks> |
Duplicate of #16143 |
Should we close this PR then for now? |
I'm not sure if this is the right change we should be making here. We had a PR like this previously which we closed: #16143 (comment) The most correct way to solve this would be to add the very most basic support of getting C# / managed code for Adding these types of conditions to our templates makes them more complicated, and requires manual intervention to undo these types of conditions if in the future we were to properly fix what this change tries to work around. |
I agree with @Redth . I think this template change is a workaround for a real fix that should go into the lower-level SDKs. If anyone needs this behavior right now they can follow the guidance in this PR in their own projects, but it's probably not something we want every new .NET 8 project to have. |
@moljac I agree with the statements from Eilon and redth. This an excellent resource for developers that need to be unblocked, so I very much appreciate you tracking this down! For now, we'll close this, and we'll see if @dalexsoto has thoughts on how we can build this into the iOS/MacCat packages for NET 9. |
I agree completely, but shouldn't that logic lead to:
??? Note: single line TFMs and no conditions for all development (not targeted) platforms. I saw linux issue, dug for few more (forgot to search for related PRs) and thought "Oh this is easy", because I am working creating my templates for repro samples. It was supposed to be quick (and temporary) fix for something community asks for. Nothing more. |
Description of Change
In project templates changed
TargetFrameworks
Conditions, so the Linux builds are green out of the box.Issues Fixed
Open
Closed