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

Set Version for Windows #15238

Merged
merged 2 commits into from
May 30, 2023
Merged

Set Version for Windows #15238

merged 2 commits into from
May 30, 2023

Conversation

mattleibow
Copy link
Member

@mattleibow mattleibow commented May 24, 2023

Description of Change

This PR fixes an issue where setting <ApplicationDisplayVersion> to anything other than 1.0 caused the IDE to fail to restore NuGets. There was a fix before #6628 however, it appears this was before another fix landed which effectively disabled the change until after the NuGet restore completed: #6767

As a result, the fix was "lost". This PR moves the fix out of the NuGet (where it is too late) and directly into the workload.

Issues Fixed

Workaround

For now the easiest workaround is to add <Version>$(ApplicationVersion)</Version> in your project directly after your <ApplicationVersion>1.0</ApplicationVersion> property.

@mattleibow mattleibow added the backport/suggested The PR author or issue review has suggested that the change should be backported. label May 24, 2023
mattleibow added a commit that referenced this pull request May 24, 2023
@mattleibow
Copy link
Member Author

Backported to #15240

Comment on lines +27 to +29
<PropertyGroup Condition=" '$([MSBuild]::GetTargetPlatformIdentifier($(TargetFramework)))' == 'windows' and '$(OutputType)' == 'WinExe' ">
<Version Condition=" $([System.Version]::TryParse ('$(ApplicationDisplayVersion)', $([System.Version]::Parse('1.0')))) ">$(ApplicationDisplayVersion)</Version>
</PropertyGroup>
Copy link
Member

Choose a reason for hiding this comment

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

This used to be in WinUI targets:

But I guess that won't work, because this would only work after a restore now:

There are 2 things different with this change than before:

  • This is in a *.Before.targets and it used to be *.After.targets. Is the ordering right?
  • The condition is a little different than what imported WinUI.targets:

<Import Project="WinUI.targets" Condition=" '$(TargetPlatformIdentifier)' == 'windows' and '$(WindowsAppSDKWinUI)' == 'true'" />

If this was moved to some *.After.targets would be be able to use the same Condition as before? $(TargetPlatformIdentifier) is probably blank at the point you have this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think using GetTargetPlatformIdentifier is the most correct way. I believe? Order does not really matter as it all comes after the workload targets - and ios/android seems to be in this spot.

'$(WindowsAppSDKWinUI)' == 'true' This is the real issue actually. when you did a PR back in the day, this was not there. I added it at some point and it broke. So now I am moving this out - and to be consistent with android and ios I am using the Before spot - which is a bad name really - it is just immediate. No idea why it is "before"...

Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

Looking at a .binlog from Essentials.DeviceTests.csproj [net7.0-windows10.0.19041] on this PR:

image
image

This looks like it matches the net7.0-android build now.

github-actions bot pushed a commit that referenced this pull request May 28, 2023
@rmarinho
Copy link
Member

/rebase

rmarinho pushed a commit that referenced this pull request May 30, 2023
@rmarinho rmarinho merged commit 130f788 into main May 30, 2023
28 of 29 checks passed
@rmarinho rmarinho deleted the dev/fix-winui-version branch May 30, 2023 11:39
@rmarinho rmarinho added the backport/approved After some discussion or review, this PR or change was approved to be backported. label May 30, 2023
mattleibow added a commit that referenced this pull request May 30, 2023
rmarinho pushed a commit that referenced this pull request May 30, 2023
* Set Version for Windows

* moved
rmarinho pushed a commit that referenced this pull request Jun 1, 2023
* Revert "Set Version for Windows (#15238)"

This reverts commit 130f788.

* Add the test

* tbis

* oops
@github-actions github-actions bot locked and limited conversation to collaborators Dec 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/approved After some discussion or review, this PR or change was approved to be backported. backport/suggested The PR author or issue review has suggested that the change should be backported. platform/windows 🪟
Projects
None yet
4 participants