Skip to content

[msbuild] Remove the SdkVersion property in the ReadAppManifest task. - #18053

Merged
rolfbjarne merged 1 commit into
dotnet:mainfrom
rolfbjarne:readappmanifest-remove-sdkversion
Apr 17, 2023
Merged

[msbuild] Remove the SdkVersion property in the ReadAppManifest task.#18053
rolfbjarne merged 1 commit into
dotnet:mainfrom
rolfbjarne:readappmanifest-remove-sdkversion

Conversation

@rolfbjarne

@rolfbjarne rolfbjarne commented Apr 13, 2023

Copy link
Copy Markdown
Member

The ReadAppManifest task doesn't need the SdkVersion, because it's only used to compute
the path to the sdk on disk, and that path can be calculated without an sdk version
now:

$ ls -la /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs
total 0
drwxrwxr-x  4 rolf  staff  128 Dec 14 10:58 .
drwxr-xr-x  5 rolf  staff  160 Nov 11 01:38 ..
drwxrwxr-x  8 rolf  staff  256 Dec 14 10:48 iPhoneOS.sdk
lrwxr-xr-x  1 rolf  staff   12 Dec 14 10:56 iPhoneOS16.2.sdk -> iPhoneOS.sdk

In fact, the path with the version just symlinks to the path without.

@rolfbjarne
[msbuild] Remove the SdkVersion property in the ReadAppManifest task.
9b4fb86 
The ReadAppManifest task doesn't need the SdkVersion, because it's only used to compute
the path to the sdk on disk, and that path can be calculated without an sdk version
now:

    $ ls -la /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs
    total 0
    drwxrwxr-x  4 rolf  staff  128 Dec 14 10:58 .
    drwxr-xr-x  5 rolf  staff  160 Nov 11 01:38 ..
    drwxrwxr-x  8 rolf  staff  256 Dec 14 10:48 iPhoneOS.sdk
    lrwxr-xr-x  1 rolf  staff   12 Dec 14 10:56 iPhoneOS16.2.sdk -> iPhoneOS.sdk

In fact, the path with the version just symlinks to the path without.
@rolfbjarne
rolfbjarne requested review from emaf and mauroa as code owners April 13, 2023 14:23
@vs-mobiletools-engineering-service2

vs-mobiletools-engineering-service2 commented Apr 13, 2023

Copy link
Copy Markdown
Collaborator

💻 [CI Build] Windows Integration Tests passed 💻

✅ All Windows Integration Tests passed.

Pipeline on Agent
Hash: 9b4fb86e802e4dd1d2ca6c234d04ff3362689e88 [PR build]

@vs-mobiletools-engineering-service2

vs-mobiletools-engineering-service2 commented Apr 13, 2023

Copy link
Copy Markdown
Collaborator

✅ API diff for current PR / commit

Legacy Xamarin (No breaking changes)
  • iOS (no change detected)
  • tvOS (no change detected)
  • watchOS (no change detected)
  • macOS (no change detected)
NET (empty diffs)
  • iOS: (empty diff detected)
  • tvOS: (empty diff detected)
  • MacCatalyst: (empty diff detected)
  • macOS: (empty diff detected)

✅ API diff vs stable

Legacy Xamarin (No breaking changes)
.NET (No breaking changes)
Legacy Xamarin (stable) vs .NET

✅ Generator diff

Generator diff is empty

Pipeline on Agent
Hash: 9b4fb86e802e4dd1d2ca6c234d04ff3362689e88 [PR build]

@vs-mobiletools-engineering-service2

vs-mobiletools-engineering-service2 commented Apr 13, 2023

Copy link
Copy Markdown
Collaborator

💻 [PR Build] Tests on macOS M1 - Mac Ventura (13.0) passed 💻

✅ All tests on macOS M1 - Mac Ventura (13.0) passed.

Pipeline on Agent
Hash: 9b4fb86e802e4dd1d2ca6c234d04ff3362689e88 [PR build]

@vs-mobiletools-engineering-service2

vs-mobiletools-engineering-service2 commented Apr 13, 2023

Copy link
Copy Markdown
Collaborator

💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻

✅ All tests on macOS M1 - Mac Big Sur (11.5) passed.

Pipeline on Agent
Hash: 9b4fb86e802e4dd1d2ca6c234d04ff3362689e88 [PR build]

@vs-mobiletools-engineering-service2

vs-mobiletools-engineering-service2 commented Apr 13, 2023

Copy link
Copy Markdown
Collaborator

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent XAMMINI-013.Ventura
Hash: 9b4fb86e802e4dd1d2ca6c234d04ff3362689e88 [PR build]

@vs-mobiletools-engineering-service2

vs-mobiletools-engineering-service2 commented Apr 13, 2023

Copy link
Copy Markdown
Collaborator

🚀 [CI Build] Test results 🚀

Test results

✅ All tests passed on VSTS: simulator tests.

🎉 All 225 tests passed 🎉

Tests counts

✅ bcl: All 69 tests passed. Html Report (VSDrops) Download
✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests: All 1 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 7 tests passed. Html Report (VSDrops) Download
✅ framework: All 8 tests passed. Html Report (VSDrops) Download
✅ generator: All 2 tests passed. Html Report (VSDrops) Download
✅ interdependent_binding_projects: All 7 tests passed. Html Report (VSDrops) Download
✅ install_source: All 1 tests passed. Html Report (VSDrops) Download
✅ introspection: All 8 tests passed. Html Report (VSDrops) Download
✅ linker: All 65 tests passed. Html Report (VSDrops) Download
✅ mac_binding_project: All 1 tests passed. Html Report (VSDrops) Download
✅ mmp: All 2 tests passed. Html Report (VSDrops) Download
✅ mononative: All 12 tests passed. Html Report (VSDrops) Download
✅ monotouch: All 25 tests passed. Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
✅ mtouch: All 1 tests passed. Html Report (VSDrops) Download
✅ xammac: All 3 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 8 tests passed. Html Report (VSDrops) Download
✅ xtro: All 2 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: 9b4fb86e802e4dd1d2ca6c234d04ff3362689e88 [PR build]

1 similar comment
@vs-mobiletools-engineering-service2

vs-mobiletools-engineering-service2 commented Apr 13, 2023

Copy link
Copy Markdown
Collaborator

🚀 [CI Build] Test results 🚀

Test results

✅ All tests passed on VSTS: simulator tests.

🎉 All 225 tests passed 🎉

Tests counts

✅ bcl: All 69 tests passed. Html Report (VSDrops) Download
✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests: All 1 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 7 tests passed. Html Report (VSDrops) Download
✅ framework: All 8 tests passed. Html Report (VSDrops) Download
✅ generator: All 2 tests passed. Html Report (VSDrops) Download
✅ interdependent_binding_projects: All 7 tests passed. Html Report (VSDrops) Download
✅ install_source: All 1 tests passed. Html Report (VSDrops) Download
✅ introspection: All 8 tests passed. Html Report (VSDrops) Download
✅ linker: All 65 tests passed. Html Report (VSDrops) Download
✅ mac_binding_project: All 1 tests passed. Html Report (VSDrops) Download
✅ mmp: All 2 tests passed. Html Report (VSDrops) Download
✅ mononative: All 12 tests passed. Html Report (VSDrops) Download
✅ monotouch: All 25 tests passed. Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
✅ mtouch: All 1 tests passed. Html Report (VSDrops) Download
✅ xammac: All 3 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 8 tests passed. Html Report (VSDrops) Download
✅ xtro: All 2 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: 9b4fb86e802e4dd1d2ca6c234d04ff3362689e88 [PR build]

mandel-macaque
mandel-macaque approved these changes Apr 14, 2023
View reviewed changes

@mandel-macaque mandel-macaque left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small nit.

Comment thread msbuild/Xamarin.MacDev.Tasks/Tasks/ReadAppManifestTaskBase.cs
// The minimum version in the Info.plist is the macOS version. However, the rest of our tooling
// expects the iOS version, so expose that.
if (!MacCatalystSupport.TryGetiOSVersion (Sdks.GetAppleSdk (Platform).GetSdkPath (SdkVersion, false), MinimumOSVersion!, out var convertedVersion, out var knownMacOSVersions))
if (!MacCatalystSupport.TryGetiOSVersion (Sdks.GetAppleSdk (Platform).GetSdkPath (string.Empty, false), MinimumOSVersion!, out var convertedVersion, out var knownMacOSVersions))

@mandel-macaque mandel-macaque Apr 14, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not an overload that just takes false, just for the code to read cleaner.

@rolfbjarne rolfbjarne Apr 17, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll do that in a different PR (since it involves another repo).

@rolfbjarne
rolfbjarne merged commit ecea1a4 into dotnet:main Apr 17, 2023
@rolfbjarne
rolfbjarne deleted the readappmanifest-remove-sdkversion branch April 17, 2023 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dalexsoto dalexsoto dalexsoto approved these changes
@mauroa mauroa mauroa approved these changes
@emaf emaf emaf approved these changes
+2 more reviewers
@mandel-macaque mandel-macaque mandel-macaque approved these changes
@haritha-mohan haritha-mohan haritha-mohan approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@rolfbjarne @vs-mobiletools-engineering-service2 @dalexsoto @mandel-macaque @mauroa @emaf @haritha-mohan