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

Use net6 previews for building #11545

Merged
merged 29 commits into from
Aug 4, 2021
Merged

Use net6 previews for building #11545

merged 29 commits into from
Aug 4, 2021

Conversation

vzarytovskii
Copy link
Member

No description provided.

azure-pipelines.yml Outdated Show resolved Hide resolved
@vzarytovskii vzarytovskii requested review from brettfo and KevinRansom and removed request for brettfo and KevinRansom May 11, 2021 09:19
@vzarytovskii vzarytovskii marked this pull request as draft May 11, 2021 11:06
@vzarytovskii
Copy link
Member Author

vzarytovskii commented May 11, 2021

Should be good after #11530 gets merged, since it has introduced searching for suffixed dotnet SDKs.

@KevinRansom KevinRansom reopened this May 11, 2021
@vzarytovskii
Copy link
Member Author

vzarytovskii commented May 11, 2021

Seems it's picking up a wrong global.json. Going to look at it tomorrow morning.

Found version 6.0.100-preview.3.21202.5 in channel 6.0 for user specified version spec: 6.0.100-preview.3.21202.5
Found version 3.1.301 in channel 3.1 for user specified version spec: 3.1.301
Found version 5.0.101 in channel 5.0 for user specified version spec: 5.0.101
No matching sdk version could be found for specified version: 666.666.666 Kindly note the preview versions are only considered in latest version searches if Include Preview Versions checkbox is checked.
Version 666.666.666 could not be found in its channel, will now search in adjacent channels.
##[error]sdk version matching: 666.666.666 could not be found 

@vzarytovskii
Copy link
Member Author

According to microsoft/azure-pipelines-tasks#11840, UseDotNet@2 will attempt to collect all the SDKs from all global.json files in the whole file tree starting from working directory.
Since we're using some global.json with wrong SDK version files in (manual) tests, it tries to install them as well and fails.
For now I will just add the version back to yaml file.

@vzarytovskii
Copy link
Member Author

vzarytovskii commented May 21, 2021

Getting some weird issue when running some of the cambridge single-test suites;

"C:\Users\vzari\code\fsharp\fsharp\artifacts\Temp\FSharp.Cambridge\5a4qpb5o.stb\e5i33gro.2iy.fsproj" (RunFSharpScript target) (1:7) ->
                   (RunFSharpScript target) -> 
                     error FS0193 : internal error : Operation is not valid due to the current state of the object.

However, whan I run the target manually, it seems to be working ok:

C:\Users\vzari\code\fsharp\fsharp\.dotnet\dotnet.exe C:\Users\vzari\code\fsharp\fsharp\artifacts\bin\fsi\Release\net5.0\fsi.dll --langversion:latest --define:NETCOREAPP --define:RELEASE --define:NET --define:NET5_0 --define:NETCOREAPP --optimize+ --warn:3 --warnaserror:3239,76 --fullpaths --flaterrors --targetprofile:netcore C:\Users\vzari\code\fsharp\fsharp\tests\fsharp\coreclr_utilities.fs C:\Users\vzari\code\fsharp\fsharp\tests\fsharp\core\array\test.fsx


C:\Users\vzari\code\fsharp\fsharp\tests\fsharp\core\array\test.fsx(862,17): warning FS1204: This function is for use by compiled F# code and should not be used directly



C:\Users\vzari\code\fsharp\fsharp\tests\fsharp\core\array\test.fsx(945,17): warning FS1204: This function is for use by compiled F# code and should not be used directly

test1 = true
test2 = true
test3 = true
test4 = true
test5 = true
test6 = true
Test Passed

@KevinRansom @brettfo does this ring a bell?

@vzarytovskii
Copy link
Member Author

vzarytovskii commented Jul 30, 2021

Okay, NativePtr.nullPtr emits ldnull for non-reference, we should just use 0n, fixed.
I guess NET6 added some additional checks/constraints.

@vzarytovskii vzarytovskii marked this pull request as ready for review July 30, 2021 16:46
@vzarytovskii
Copy link
Member Author

dotnet/runtime#54656 gonna end up in preview7, so this is blocked for now from merging, gonna update and see if it's building and running tests fine.

@vzarytovskii
Copy link
Member Author

After talking to @KevinRansom and @brettfo disabling failing test as agreed, going to re-enable after preview7 is released.

@vzarytovskii vzarytovskii reopened this Aug 3, 2021
@vzarytovskii
Copy link
Member Author

@KevinRansom @brettfo We can merge it now (fsi array tests are disabled, going to re-enable once preview7 is released).

Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

Looks good, why the deleted span tests?

tests/fsharp/Compiler/Language/SpanTests.fs Outdated Show resolved Hide resolved
tests/fsharp/tools/eval/test.fsx Show resolved Hide resolved
eng/Build.ps1 Outdated Show resolved Hide resolved
@vzarytovskii
Copy link
Member Author

Looks good, why the deleted span tests?

These were testing a regression, which got fixed with net6.

On a second thought I probably should just change the assert.

@vzarytovskii vzarytovskii merged commit 8169f2c into dotnet:main Aug 4, 2021
@vzarytovskii vzarytovskii deleted the net6 branch August 4, 2021 11:54
@auduchinok
Copy link
Member

auduchinok commented Aug 9, 2021

@vzarytovskii What is the reason to require the SDK preview for building FSharp.Compiler.Service.sln? It makes things much worse for external contributors.

@vzarytovskii
Copy link
Member Author

vzarytovskii commented Aug 9, 2021

@vzarytovskii What is the reason to require the SDK preview for building FSharp.Compiler.Service.sln? It makes things much worse for external contributors.

We use sdk Arcade uses by default, which will allow us to test latest runtime/sdk faster and see if there were any regressions (we were hitting some earlier this year).

It is a fair point though, I'm not sure how can we support more than one SDK.

@auduchinok
Copy link
Member

It is a fair point though, I'm not sure how can we support more than one SDK.

We can move the FCS solution back to a separate folder and have a separate global.json there.

@vzarytovskii
Copy link
Member Author

vzarytovskii commented Aug 9, 2021

It is a fair point though, I'm not sure how can we support more than one SDK.

We can move the FCS solution back to a separate folder and have a separate global.json there.

Yeah, I guess that would work, it bothers me slightly though - that we'll essentially use two different SDKS.

What SDK do you think should be used for the service?

@auduchinok
Copy link
Member

auduchinok commented Aug 9, 2021

What SDK do you think should be used for the service?

I'd say the oldest suitable for building the solution (including on the CI) or the previously used one, 5.0.300.

@vzarytovskii
Copy link
Member Author

What SDK do you think should be used for the service?

I'd say the oldest suitable for building the solution (including on the CI) or the previously used one, 5.0.300.

Huh, I don't think we build service solution in CI (at least, not directly via build scripts, or in yml), probably should add those too.

@vzarytovskii
Copy link
Member Author

Created #11933 for tracking it.

@auduchinok
Copy link
Member

probably should add those too.

That would be really great if it's indeed not being built currently! 🙂

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

Successfully merging this pull request may close these issues.

4 participants