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

Change devcontainer to use image+features #16335

Merged

Conversation

vzarytovskii
Copy link
Member

So we don't have to take care about updates for Debian, etc.

Copy link
Member

@baronfel baronfel left a comment

Choose a reason for hiding this comment

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

I really like the way features make things more composable - always thought the old Dockerfile-based method, while allowing control, was too hard to keep updated!

.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
@vzarytovskii vzarytovskii marked this pull request as ready for review November 30, 2023 19:14
@vzarytovskii vzarytovskii requested a review from a team as a code owner November 30, 2023 19:14
@vzarytovskii
Copy link
Member Author

Any objections against merging this? @Martin521 i know you use devcontainers a bunch.

@Martin521
Copy link
Contributor

Thanks for asking. I trust you are doing the right thing, but if it can wait another day, I will try it out here.

@vzarytovskii
Copy link
Member Author

Thanks for asking. I trust you are doing the right thing, but if it can wait another day, I will try it out here.

Absolutely. That'd be great if you can try and see if it still works for you, and nothing is (more) broken.

@Martin521
Copy link
Contributor

Martin521 commented Dec 1, 2023

Works all fine for me except that the test discovery of the Ionide test explorer fails.
It took me some time to trace it down.
The error message Couldn't build test projects. Make sure you can build projects with 'dotnet build' in the "Test Explorer" window was misleading. Looking into the sources, I found that the test discovery first calls msbuild /t:Build for the three test projects. Which led me to finding the following build output in the `Ionide msbuild" window:

[11:42:39 INFO ] [msbuild] invoking msbuild from /usr/bin/dotnet on /workspaces/fsharp3/tests/FSharp.Compiler.UnitTests/FSharp.Compiler.UnitTests.fsproj for target Build
MSBuild version 17.8.0-preview-23418-03+0125fc9fb for .NET
/usr/share/dotnet/sdk/8.0.100-rc.1.23463.5/Sdks/Microsoft.NET.Sdk/targets/Microsoft.PackageDependencyResolution.targets(266,5): error NETSDK1004: Assets file '/workspaces/fsharp3/artifacts/obj/FSharp.Compiler.UnitTests/project.assets.json' not found. Run a NuGet package restore to generate this file. [/workspaces/fsharp3/tests/FSharp.Compiler.UnitTests/FSharp.Compiler.UnitTests.fsproj::TargetFramework=net8.0]
[11:42:41 INFO ] [msbuild] invoking msbuild from /usr/bin/dotnet on /workspaces/fsharp3/tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.Tests.fsproj for target Build
MSBuild version 17.8.0-preview-23418-03+0125fc9fb for .NET
/usr/share/dotnet/sdk/8.0.100-rc.1.23463.5/Sdks/Microsoft.NET.Sdk/targets/Microsoft.PackageDependencyResolution.targets(266,5): error NETSDK1004: Assets file '/workspaces/fsharp3/artifacts/obj/FSharp.Compiler.Service.Tests/project.assets.json' not found. Run a NuGet package restore to generate this file. [/workspaces/fsharp3/tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.Tests.fsproj::TargetFramework=net8.0]
[11:42:44 INFO ] [msbuild] invoking msbuild from /usr/bin/dotnet on /workspaces/fsharp3/tests/FSharp.Compiler.ComponentTests/FSharp.Compiler.ComponentTests.fsproj for target Build
MSBuild version 17.8.0-preview-23418-03+0125fc9fb for .NET
  FSharp.Core -> /workspaces/fsharp3/artifacts/bin/FSharp.Core/Debug/netstandard2.0/FSharp.Core.dll
  FSharp.Compiler.Interactive.Settings -> /workspaces/fsharp3/artifacts/bin/FSharp.Compiler.Interactive.Settings/Debug/netstandard2.0/FSharp.Compiler.Interactive.Settings.dll
  FSharp.DependencyManager.Nuget -> /workspaces/fsharp3/artifacts/bin/FSharp.DependencyManager.Nuget/Debug/netstandard2.0/FSharp.DependencyManager.Nuget.dll
  FSharp.Compiler.Service -> /workspaces/fsharp3/artifacts/bin/FSharp.Compiler.Service/Debug/netstandard2.0/FSharp.Compiler.Service.dll
  FSharp.Core -> /workspaces/fsharp3/artifacts/bin/FSharp.Core/Debug/netstandard2.1/FSharp.Core.dll
  FSharp.Test.Utilities -> /workspaces/fsharp3/artifacts/bin/FSharp.Test.Utilities/Debug/net8.0/FSharp.Test.Utilities.dll
  FSharp.Compiler.ComponentTests -> /workspaces/fsharp3/artifacts/bin/FSharp.Compiler.ComponentTests/Debug/net8.0/FSharp.Compiler.ComponentTests.dll

So msbuild complains about a missing project.assets.json for UnitTests and Service.Tests, while ComponentTests build ok.

I found that setting BUILDING_USING_DOTNET=true in devcontainer.json solves the issue, but I don't know if that is the best way to do it.
Or if some setting in one of the msbuild props/proj files can solve it.
Or if Ionide test explorer should just use 'dotnet build' (but I am not sure how soon that can happen).

@vzarytovskii
Copy link
Member Author

vzarytovskii commented Dec 1, 2023

Yeah, if we set BUILDING_USING_DOTNET=true for dev container, then it will be impossible to build using build scripts. I don't know if it's possible to set this property to only dotnet commands in the test explorer.

@Martin521
Copy link
Contributor

I don't know if it's possible to set this property to only dotnet commands in the test explorer.

There is no such setting.
Env seems to be the only way, and since VS Code is started from the dev container, it would need to be there.

@vzarytovskii
Copy link
Member Author

I don't know if it's possible to set this property to only dotnet commands in the test explorer.

There is no such setting. Env seems to be the only way, and since VS Code is started from the dev container, it would need to be there.

It seems so, yeah. Problem is that in this case, build scripts won't work if someone needs them.

@Martin521
Copy link
Contributor

Ok, then I guess you should proceed with this PR.
I will file an issue for Ionide and in the meantime use Directory.build.props.user to set BUILDING_USING_DOTNET.

@psfinaki
Copy link
Member

psfinaki commented Dec 6, 2023

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@psfinaki
Copy link
Member

psfinaki commented Dec 6, 2023

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@vzarytovskii vzarytovskii merged commit dc40139 into dotnet:main Dec 7, 2023
26 checks passed
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