-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Clean up OS-related property uses, update buildtools, update MSBuild #15728
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
1.0.27-prerelease-01230-01 | ||
1.0.27-prerelease-01302-01 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -246,10 +246,8 @@ | |
</Target> | ||
|
||
<Target Name="CheckTestPlatforms"> | ||
<GetTargetMachineInfo Condition="'$(TargetOS)' == ''"> | ||
<Output TaskParameter="TargetOS" PropertyName="TargetOS" /> | ||
</GetTargetMachineInfo> | ||
<PropertyGroup> | ||
<TargetOS Condition="'$(TargetOS)' == ''">$(DefaultOSGroup)</TargetOS> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't enough to correctly filter for the other OS's. DefaultOSGroup will only ever have OSX, Unix, or Windows_NT. I expect this will break some tests. See TargetOSTrait above for example it has some other targets. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about FreeBSD/NetBSD? will those ever be set? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They will not, but we don't use those anywhere right now. We'd probably need to add some extra logic to auto-detect it, or go back to using the build task for that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK can you clean those out then if they are dead. I would like to keep these things consistent There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's fine to keep them. There are a couple of tests that have |
||
<TestDisabled Condition="'%(UnsupportedPlatformsItems.Identity)' == '$(TargetOS)' Or '$(ConfigurationErrorMsg)' != ''">true</TestDisabled> | ||
</PropertyGroup> | ||
<Message Text="CheckTestPlatforms found TargetOS of [$(TargetOS)]." Importance="Low" /> | ||
|
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.
@mellinoe noticed this while looking at the other problem: there are still a few targets in buildtools that rely on OsEnvironment being set, e.g.: https://github.com/dotnet/buildtools/search?utf8=%E2%9C%93&q=OsEnvironment
I think with this change those would be doing the wrong thing.
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 there are a few things in there that would be broken, although most of the interesting ones are being overridden here. I'll file an issue in buildtools to clean those up when we integrate the corefx changes back into there.
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.
dotnet/buildtools#1323