Fix OS condition#5802
Conversation
The `$(OS)` property on Linux machines is `Unix` not `Linux`. This was causing builds to fail on Linux. Flipped to making it a positive check for Windows since it looks like none of this should fire for non-Windows.
jaredpar
requested review from
FabianMeiswinkel,
adityasa,
khdang,
kirankumarkolli,
kirillg,
kushagraThapar,
neildsh and
sboshra
as code owners
|
Azure Pipelines: 2 pipeline(s) require an authorized user to comment /azp run to run. |
| </ItemGroup> | ||
|
|
||
| <ItemGroup Condition="$(OS) != 'Linux' AND '$(ProjectRef)' != 'True' "> | ||
| <ItemGroup Condition="$(OS) == 'Windows_NT' AND '$(ProjectRef)' != 'True' "> |
There was a problem hiding this comment.
This means that this will no longer run on macOS, right? I think that's correct, but want to call it out explicitly.
There was a problem hiding this comment.
Correct. That seemed to be the intent here. Also MSBuild doesn't have a great way to differentiate between Linux and MacOS. The $(OS) value for both is "Unix". Can differentiate but you have to use a different type of condition
There was a problem hiding this comment.
Since the original intent was not honored and so-far not know of any broken/impact. How about remove condition it-self?
There was a problem hiding this comment.
Well, @jaredpar was trying to build on Linux and it didn't work, right? So presumably something about that doesn't work on Linux?
There was a problem hiding this comment.
/usr/lib/dotnet/sdk/10.0.106/Microsoft.Common.CurrentVersion.targets(5381,5): error MSB3030: Could not copy the file "/home/kirankk/.nuget/packages/Microsoft.HybridRow/1.1.0-preview3/microsoft.hybridrow.nuspec" because it was not found.
Yup. lets take this PR then.
There was a problem hiding this comment.
Seems this was primarily driven through the yml files where its set.
One option might be <ItemGroup Condition="$(TargetFramework.Contains('windows'))"> (for build time?)
There was a problem hiding this comment.
WSL varified that it works
There was a problem hiding this comment.
Yeah I was trying to review the other PR and I didn't quite understand how the build laid out files such that the targets could actually work. Opened up the repo in codespaces to play around and the build failed. The fix was essentially just changing the condition after which build worked fine.
|
@kirankumarkolli - can you take a look? |
3 participants
The
$(OS)property on Linux machines isUnixnotLinux. This was causing builds to fail on Linux. Flipped to making it a positive check for Windows since it looks like none of this should fire for non-Windows.Pull Request Template
Description
Please include a summary of the change and which issue is fixed. Include samples if adding new API, and include relevant motivation and context. List any dependencies that are required for this change.
Type of change
Please delete options that are not relevant.
Closing issues
To automatically close an issue: closes #IssueNumber