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

EditorFeatures.Cocoa: Build with net6.0 #59429

Merged
merged 24 commits into from
Mar 18, 2022
Merged

Conversation

sandyarmstrong
Copy link
Member

@sandyarmstrong sandyarmstrong commented Feb 9, 2022

Use macos workload 12.1.200-preview.13.7. Necessitates bumping the editor and several other packages to 17.2 (17.1.387 might also be OK for editor).

  • Build with public editor packages
  • Install proper macos workload version
  • Local builds working
  • Build_Windows* CI jobs working
  • Source-Build CI jobs working
  • Correctness_Determinism CI jobs working (need to ensure macos workload installed in this scenario)
  • Adapt workload installation script for bash (unless we can use pwsh in unix pipelines?)

@sandyarmstrong
Copy link
Member Author

@KirillOsenkov @Therzok @davidwengier @tmat jfyi. I'm not sure exactly when this will be ready for review. We'll likely need it for your 17.2.

@davidwengier
Copy link
Contributor

main is targetting 17.2 so this is good on that front. I'm not planning another VS Mac insertion until after Feb 25 due to scheduling (and realistically not till after March 6 because I'll be on holidays) so there is plenty of time.

@sandyarmstrong sandyarmstrong force-pushed the dev/sandy/cocoa-net6 branch 2 times, most recently from 8e417c7 to e3cbe65 Compare February 17, 2022 05:15
@sandyarmstrong
Copy link
Member Author

sandyarmstrong commented Feb 17, 2022

##[error]C:\Program Files\Microsoft Visual Studio\2022\Preview\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.RestoreEx.targets(19,5): error : (NETCORE_ENGINEERING_TELEMETRY=Restore) The expression "[MSBuild]::MakeRelative(D:\a_work\1\s\src\EditorFeatures\Core.Cocoa, *.plist)" cannot be evaluated. Illegal characters in path. D:\a_work\1\s.dotnet\packs\Microsoft.macOS.Sdk\12.1.302-preview.14.6\targets\Microsoft.macOS.Sdk.DefaultItems.props

https://github.com/xamarin/xamarin-macios/blob/main/dotnet/targets/Microsoft.Sdk.DefaultItems.template.props#L23

I wonder what this is about. Anyway, I'll need to fix that P14 of the macos workload is being used instead of P13.

@sandyarmstrong
Copy link
Member Author

I used a backport of this PR to bring this into VSMac.

To finish this PR, I need to get a few editor packages published. Should be able to get that done next week after some signing infrastructure issues are addressed.

@sandyarmstrong sandyarmstrong force-pushed the dev/sandy/cocoa-net6 branch 2 times, most recently from 30fa881 to f40f22c Compare March 1, 2022 20:48
eng/build.ps1 Outdated Show resolved Hide resolved
@sandyarmstrong sandyarmstrong force-pushed the dev/sandy/cocoa-net6 branch 2 times, most recently from 3c6cceb to 9cf9d71 Compare March 2, 2022 21:16
@sandyarmstrong sandyarmstrong force-pushed the dev/sandy/cocoa-net6 branch 2 times, most recently from 2f93672 to a28402a Compare March 3, 2022 00:52
@sandyarmstrong
Copy link
Member Author

sandyarmstrong commented Mar 3, 2022

I had previously done work inside eng\common to ensure that initializing a .NET "Core" installation also initializes workloads. I am now trying to keep all changes out of there to avoid having a dependency on a newer Arcade. This may mean having to sprinkle workload initialization in a couple of extra places.

In the long run, workload management may move to be a common Arcade feature.

eng/Versions.props Outdated Show resolved Hide resolved
@sandyarmstrong sandyarmstrong marked this pull request as ready for review March 3, 2022 02:14
@sandyarmstrong sandyarmstrong requested review from a team as code owners March 3, 2022 02:14
eng/build.sh Outdated Show resolved Hide resolved
eng/Versions.props Outdated Show resolved Hide resolved
global.json Outdated Show resolved Hide resolved
eng/Install-DotNetWorkloads.ps1 Outdated Show resolved Hide resolved
sandyarmstrong and others added 13 commits March 16, 2022 14:04
In CI, we keep seeing errors like:

```
Workload installation failed: Failed to validate package signing.

Verifying Microsoft.NET.Runtime.MonoAOTCompiler.Task.6.0.2-mauipre.1.22102.15

error: NU3004: The package is not signed.

Package signature validation failed.
```

This is presumably happening because NuGet.config references multiple
feeds, some of which have signed versions of workload packages, and some
of which have unsigned versions.

We can untangle this later. For now, change EditorFeatures.Cocoa to
target `net6.0` instead of `net6.0-macos`, and manually reference the
assemblies it needs from the macos SDK and the VS editor.
@davidwengier
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@sandyarmstrong
Copy link
Member Author

When I rebased yesterday I think there was only one failure. After your /azp run there are more. Do we just /azp run until it turns green?

@davidwengier
Copy link
Contributor

davidwengier commented Mar 17, 2022

Odd. When I did the azp run I'm 90% sure all of the various integration tests were failing. If it was just one, I normally just re-run that one /azp run <leg name>

Generally speaking, yes we're in a bit of a "re-run until they turn green" situation, though with new editor APIs it is worth at least checking if the failures are consistent, and investigating. Looks like the GoToBaseFromMetadataAsSource test failure could be real. Will have a look and see what i can work out. @sharwell don't suppose you can quickly check the history and see if any of your recent fixes are missing from main-vs-deps?

@davidwengier
Copy link
Contributor

The stack in the Go To Base test failure seems to imply an issue with the shell:

System.ArgumentException : Value does not fall within the expected range.
   at System.Runtime.InteropServices.Marshal.ThrowExceptionForHRInternal(Int32 errorCode, IntPtr errorInfo)
   at Microsoft.VisualStudio.Extensibility.Testing.ShellInProcess.<ExecuteCommandAsync>d__10.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Microsoft.VisualStudio.Extensibility.Testing.EditorInProcess.<GoToBaseAsync>d__48.MoveNext() in /_/src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/EditorInProcess.cs:line 965
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at Roslyn.VisualStudio.NewIntegrationTests.CSharp.CSharpGoToBase.<GoToBaseFromMetadataAsSource>d__3.MoveNext() in /_/src/VisualStudio/IntegrationTest/New.IntegrationTests/CSharp/CSharpGoToBase.cs:line 39
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.GetResult()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)

It runs fine and passes on my machine, but that probably doesn't mean much. @sharwell any ideas? Is this the first test run with the new Guid attribute being applied to the EditorConstants.EditorCommandID type? Though if that was causing a failure, I would expect it to fail locally for me too.

@davidwengier
Copy link
Contributor

Okay, everything is green.. stand back, I'm gonna merge it!

@davidwengier davidwengier merged commit a83e2ad into main-vs-deps Mar 18, 2022
@davidwengier davidwengier deleted the dev/sandy/cocoa-net6 branch March 18, 2022 03:08
@ghost ghost added this to the Next milestone Mar 18, 2022
@sandyarmstrong
Copy link
Member Author

Thanks David!

@allisonchou allisonchou modified the milestones: Next, 17.2.P3 Mar 28, 2022
@RikkiGibson
Copy link
Contributor

What version of VS is needed to work with these changes? Trying to figure out the expected lifetime of the -vs-deps branch here.

@KirillOsenkov
Copy link
Member

you mean VSWin or VSMac?

@RikkiGibson
Copy link
Contributor

Just VSWin, I think. Once a public preview comes out which can F5 successfully on whatever we have in main-vs-deps, we'd like to shut down the -vs-deps branch and go back to just shipping out of main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants