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

upgrade to version 6 of the Arcade sdk #51647

Closed
wants to merge 7 commits into from

Conversation

jmarolf
Copy link
Contributor

@jmarolf jmarolf commented Mar 4, 2021

No description provided.

@jmarolf jmarolf requested a review from a team as a code owner March 4, 2021 00:29
@dibarbet
Copy link
Member

dibarbet commented Mar 4, 2021

From the thread I see, the suggestion was to use arcade 6 due to lts support which 5 didn't have, any reason not to do 6?

@jmarolf
Copy link
Contributor Author

jmarolf commented Mar 4, 2021

I just want to confirm that the CI succeeds for 5 first. upgrading to 6 is trivial if folks decide we should

@jmarolf
Copy link
Contributor Author

jmarolf commented Mar 4, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@jmarolf jmarolf force-pushed the infrastructure/move-to-arcade-5 branch from db96f28 to ddbc3be Compare March 4, 2021 03:51
@jaredpar
Copy link
Member

jaredpar commented Mar 4, 2021

Problem is we specifically cannot take 5 because it doesn't meet our servicing requirements. If it's trivial to do 6 then why aren't we starting with upgrading to 6?

@jmarolf
Copy link
Contributor Author

jmarolf commented Mar 4, 2021

Problem is we specifically cannot take 5 because it doesn't meet our servicing requirements. If it's trivial to do 6 then why aren't we starting with upgrading to 6?

6 will require us to use .NET 6 SDK Preview 1. Thats a big ask on the team (and contributors). I've updated to Arcade 6 to see what it looks like. Appears that there is a VB compiler bug I need to dig into.

##[error]src/Compilers/Test/Utilities/VisualBasic/CompilationTestUtils.vb(1217,22): error BC30455: Argument not specified for parameter 'Number' of 'Public Function Str(Number As Object) As String'.

@RikkiGibson
Copy link
Contributor

That looks like the same oddity we were seeing in #51596

@jaredpar
Copy link
Member

jaredpar commented Mar 4, 2021

Appears that there is a VB compiler bug I need to dig into.

That's not a VB compiler bug but a sign that a bad version of MS.VB.dll is being used.

@jmarolf
Copy link
Contributor Author

jmarolf commented Mar 4, 2021

That's not a VB compiler bug

Its never a compiler bug :)

@RikkiGibson
Copy link
Contributor

I popped open the Microsoft.VisualBasic.dlls for net472 in both the 1.0.0 and 1.0.0-preview1 ref assemblies packages. The member Microsoft.VisualBasic.Conversion.Str(object) is present in both and appears identical.

I don't know what else we should try to track this down. It does look to me like we need to do something about this, though.

@jaredpar
Copy link
Member

jaredpar commented Mar 4, 2021

@RikkiGibson

The member Microsoft.VisualBasic.Conversion.Str(object) is present in both and appears identical

Guessing this specific error message is a red herring. A lot like when overload resolution fails and the compiler spits out the error message in relation to the "best option".

Consider that we don't actually use Str in the compiled code at all as evidence here.

I don't know what else we should try to track this down.

Think you'd need to find the real error here. What other error could possibly cause VB to fall back to the Str helper? That is likely the real problem.

It does look to me like we need to do something about this, though

I'd put the urgency on this low. The current use of the preview-1 reference assemblies works fine. I'm not sure why we're conflating it with a move to Arcade 6.

@RikkiGibson
Copy link
Contributor

The current use of the preview-1 reference assemblies works fine.

It seems that the new netframework reference assemblies are coming along for the ride implicitly when moving to arcade 6. So perhaps we should figure out why the new version is getting picked up instead of our version. Maybe a property in Versions.Props has been renamed on the Arcade side, for example.

@jaredpar
Copy link
Member

jaredpar commented Mar 4, 2021

Maybe a property in Versions.Props has been renamed on the Arcade side, for example.

True. Seems probable.

If that is the single VB error though we could also just focus on figuring out what that is and moving past it. It's ringing a bell deep in my brain but having trouble paging it in at the moment.

@jaredpar
Copy link
Member

jaredpar commented Mar 4, 2021

It's ringing a bell deep in my brain but having trouble paging it in at the moment.

Paging operation complete. One of the following is happening:

  1. The new arcade is adding new global Import statements to MS.VB projects, one of which is Microsoft.VisualBasic
  2. The new arcade is adding a reference to a version of Microsoft.VisualBasic which wasn't there before

The result though is str here is being treated like the Module member Microsoft.VisualBasic.Conversions.Str not an identifier which is how we're reading it.

Simple repro of the problem is here. Delete the Import Microsoft.VisualBasic and the code will compile

https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQBsQDdhJiAligHwEkwAHAewRigAIBlAT1jjAFgAoEiq2gWVwDGCclHIAzGADoAarigBXCCgBC0QRw69yAE3ko4NXhxomaABXnAUgmgDF5AOwExc5B/UowAgg+29EAOZwdDAIuA4BUAAUZhAIEGBeCPEMNLBhEbRetCEZAVEAlAU02fSh4QHGptVJKZJ0nlHpFVAFGuzV1baUNACiEAIAFmmhNETuzZlVnZ0Awm6i+pIA6mHwADLhcE2hbR00050AcnAAHjDt1QhwMPII7kfkMIMVh72+do7Org4c79qGHR6OBAA

@RikkiGibson
Copy link
Contributor

@jmarolf was just telling me, IIRC, that when the build succeeds it is pulling MS.VB.dll from the system's framework folder, and when it fails it's pulling from the NuGet package. Perhaps examining the package references will shed a little light?

@jmarolf
Copy link
Contributor Author

jmarolf commented Mar 4, 2021

@jaredpar so I assume the fix is to update arcade to not import this automatically?

@jaredpar
Copy link
Member

jaredpar commented Mar 4, 2021

when the build succeeds it is pulling MS.VB.dll from the system's framework folder

What is the system folder in this case?

so I assume the fix is to update arcade to not import this automatically?

Don't know. I would take a quick look and see how it's being built today. Build that assembly, crack open the binary log and see what global Import are being specified and what references are being passed. Then do the same for Arcade 6 so we know what's happening here.

@jmarolf jmarolf force-pushed the infrastructure/move-to-arcade-5 branch 3 times, most recently from c63e260 to 70947ac Compare March 5, 2021 05:58
@jmarolf jmarolf changed the title upgrade to version 5 of the Arcade sdk upgrade to version 6 of the Arcade sdk Mar 5, 2021
@jmarolf
Copy link
Contributor Author

jmarolf commented Mar 5, 2021

Alright, after a brief sojourn with Dante, I think this is working now. Queued a signed build to verify nothing else is horribly broken

@jmarolf jmarolf force-pushed the infrastructure/move-to-arcade-5 branch from 834e23c to 9af8a16 Compare March 5, 2021 08:25
@jaredpar
Copy link
Member

jaredpar commented Mar 5, 2021

What was the resolution of the str issue in the VB projects?

@jmarolf
Copy link
Contributor Author

jmarolf commented Mar 5, 2021

What was the resolution of the str issue in the VB projects?

We were getting moved to version 1.0 of Microsoft.NETFramework.ReferenceAssemblies instead of 1.0.0-preview.1

@jmarolf jmarolf force-pushed the infrastructure/move-to-arcade-5 branch from 9af8a16 to 275b510 Compare March 5, 2021 21:21
@jmarolf
Copy link
Contributor Author

jmarolf commented Mar 5, 2021

It seems updating to arcade 6 somehow causes us to run headlong into the brick wall that is #48816. @sharwell if you want a 100% local repo here is it. How have you been investigating these hangs?

@jaredpar
Copy link
Member

jaredpar commented Mar 5, 2021

We were getting moved to version 1.0 of Microsoft.NETFramework.ReferenceAssemblies instead of 1.0.0-preview.1

Yes but why is that an issue? The MS.VB.dll in 1.0.0 and 1.0.0-preview.1 is basically the same from what I can see. Both have Conversions.Str and hence that should not explain the issue here.

@jmarolf
Copy link
Contributor Author

jmarolf commented Mar 6, 2021

Yes but why is that an issue? The MS.VB.dll in 1.0.0 and 1.0.0-preview.1 is basically the same from what I can see. Both have Conversions.Str and hence that should not explain the issue here.

I have the commandline args to vbc and its a smoking gun. Pass 1.0 reference assemblies get the error, pass preview and build succeeds. The fix was to pin the preview versions and the build succeeds. There are two follow up items here:

@jmarolf
Copy link
Contributor Author

jmarolf commented Mar 10, 2021

Currently failing due to

Compilation error(s) occurred:  Cannot use file stream for [C:\h\w\A86A091F\w\BD710A50\e\testhost.deps.json]: No such file or directory
A fatal error was encountered. The library 'hostpolicy.dll' required to execute the application was not found in 'C:\h\w\A86A091F\p\dotnet-cli'.
Failed to run as a self-contained app.

The application was run as a self-contained app because 'C:\h\w\A86A091F\w\BD710A50\e\testhost.runtimeconfig.json' was not found.
If this should be a framework-dependent app, add the 'C:\h\w\A86A091F\w\BD710A50\e\testhost.runtimeconfig.json' file and specify the appropriate framework.
Expected: False
Actual:   True

I'll need to investigate why testhost.deps.json isn't being deployed

@MichaelSimons
Copy link
Member

@jmarolf, Friendly ask if there are any updates?

@JoeRobich JoeRobich mentioned this pull request Mar 30, 2021
@jmarolf
Copy link
Contributor Author

jmarolf commented Mar 30, 2021

superseded by #52255

@jmarolf jmarolf closed this Mar 30, 2021
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.

5 participants