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.

Sign up for GitHub

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

Jump to bottom

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 •
edited
Loading

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
Backport of #15238
8031ca2
@mattleibow mattleibow mentioned this pull request May 24, 2023
Merged
@mattleibow
Copy link
Member Author

Backported to #15240

jonathanpeppers
jonathanpeppers reviewed May 24, 2023
View reviewed changes
src/Workload/Microsoft.Maui.Sdk/Sdk/Microsoft.Maui.Sdk.Before.targets
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:

maui/src/Workload/Microsoft.Maui.Sdk/Sdk/Microsoft.Maui.Core.Sdk.After.targets

Line 14 in 8b8293f

<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"...

jonathanpeppers
jonathanpeppers approved these changes May 24, 2023
View reviewed changes
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
@mattleibow @rmarinho
Backport of #15238
6b9fab7
@rmarinho
Copy link
Member

/rebase

rmarinho pushed a commit that referenced this pull request May 30, 2023
@mattleibow
Backport of #15238 (#15240)
a860fa9
@rmarinho rmarinho merged commit 130f788 into main May 30, 2023
@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
@mattleibow
Revert "Set Version for Windows (#15238)"
c5d8837 
This reverts commit 130f788.
@mattleibow mattleibow mentioned this pull request May 30, 2023
Merged
rmarinho pushed a commit that referenced this pull request May 30, 2023
@mattleibow @rmarinho
Set Version for Windows (#15238)
df49ad9 
* Set Version for Windows

* moved
rmarinho pushed a commit that referenced this pull request Jun 1, 2023
@mattleibow
Add some tests for changing the app version properties (#15338)
bd31a9a 
* 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
@samhouts samhouts added the fixed-in-8.0.0-preview.5.8529 Look for this fix in 8.0.0-preview.5.8529! label Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rmarinho rmarinho rmarinho approved these changes

@jonathanpeppers jonathanpeppers jonathanpeppers approved these changes

@Redth Redth Awaiting requested review from Redth

Assignees
No one assigned
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. fixed-in-8.0.0-preview.5.8529 Look for this fix in 8.0.0-preview.5.8529! platform/windows 🪟
Projects
None yet
Milestone
No milestone
4 participants
@mattleibow @rmarinho @jonathanpeppers @samhouts