Skip to content

Proof of concept using different base paths #2079

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+36 −10
Draft

Proof of concept using different base paths #2079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions src/Components/MSBuild.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ module MSBuild =
| Ok msbuild -> Promise.lift msbuild
| Error msg -> Promise.reject (exn msg))

let invokeMSBuildWithCancel project target (cancellationToken: CancellationToken) =
let invokeMSBuildWithCancel project target (cancellationToken: CancellationToken) args =
Copy link
Member Author

Choose a reason for hiding this comment

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

We're doing the classic calling msbuild t:Build here instead of dotnet build which can cause issues with needing to restore. This is currently in a hacky state so we can discuss options.

Copy link
Contributor

Choose a reason for hiding this comment

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

on thing to consider is that /t:Build calls the build target - and not all projects will have that target. Most managed projects will, but what dotnet build and msbuild do by default is call the default target so the most correct thing to do might be to allow specifying an optional target?

you can also do dotnet build with --no-restore if you want to skip the implicit restore.


if cancellationToken.isCancellationRequested then
promise { return { Code = None; Signal = Some "SIGINT" } }
Expand All @@ -33,12 +33,17 @@ module MSBuild =
let cfg = workspace.getConfiguration ()
cfg.get ("FSharp.msbuildAutoshow", false)

let command = [ project; $"/t:%s{target}" ]
let command =
[
project;
// $"/t:%s{target}"
]
@ args

let executeWithHost () =
promise {
let! msbuildPath = dotnetBinary ()
let cmd = ResizeArray("msbuild" :: command)
let cmd = ResizeArray("build" :: command)
logger.Info("invoking msbuild from %s on %s for target %s", msbuildPath, project, target)

if autoshow then
Expand Down Expand Up @@ -222,7 +227,7 @@ module MSBuild =
}
}

let buildProjectPath target (project: Project) = invokeMSBuild project.Project target
let buildProjectPath target (project: Project) = invokeMSBuild project.Project target []

let buildProjectPathFast (project: Project) =
promise {
Expand Down Expand Up @@ -252,7 +257,7 @@ module MSBuild =

p.report pm

invokeMSBuild path "Restore" |> Promise.bind continuation |> Promise.toThenable)
invokeMSBuild path "Restore" [] |> Promise.bind continuation |> Promise.toThenable)
)
|> Promise.ofThenable
|> Async.AwaitPromise
Expand Down
31 changes: 26 additions & 5 deletions src/Components/TestExplorer.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,21 @@ module CancellationToken =

tokenSource.token



type TestId = string
type ProjectPath = string
type TargetFramework = string

module Project =
let testPathSubDir = ".ionide-test"
Copy link
Contributor

Choose a reason for hiding this comment

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

idea: consider using ExtensionContext.storageUri from the activate call to provide our workspace-unique state path?

let getOutputPaths () =
let objOutputPath = node.path.join [| "obj" ; testPathSubDir ; node.path.sep|]
Copy link
Member Author

Choose a reason for hiding this comment

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

Using relative paths means it can build each respective project in its own subfolder (Real lib and test project), otherwise, things like project.assets.json will collide. . Though I'm not entirely sure this is the correct approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

using separate subfolders is the right move here IMO

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the same discussion that's going on for the dotnet run foo.cs work, and separate intermediate/output paths is what that effort has landed on as well.

let baseIntermediateOutputPath = $"/p:BaseIntermediateOutputPath={objOutputPath}"
let binOutputPath = node.path.join [| "bin" ; testPathSubDir; node.path.sep |]
let baseOutputPath = $"/p:BaseOutputPath={binOutputPath}"
[baseIntermediateOutputPath; baseOutputPath]

module ProjectPath =
let inline ofString str = str

Expand Down Expand Up @@ -486,8 +497,9 @@ module DotnetCli =

let restore
(projectPath: string)
args
: JS.Promise<Node.ChildProcess.ExecError option * StandardOutput * StandardError> =
Process.exec "dotnet" (ResizeArray([| "restore"; projectPath |]))
Process.exec "dotnet" (ResizeArray([| "restore"; projectPath; yield! args |]))

let private debugProcessIdRegex = RegularExpressions.Regex(@"Process Id: (.*),")

Expand Down Expand Up @@ -621,14 +633,16 @@ module DotnetCli =
promise {
let additionalArgs = if not shouldBuild then [| "--no-build" |] else Array.empty

let basePathArgs = Project.getOutputPaths ()

let! _, stdOutput, _ =
dotnetTest
cancellationToken
projectPath
targetFramework
None
NoDebug
[| "--list-tests"; yield! additionalArgs |]
[| "--list-tests"; yield! additionalArgs; yield! basePathArgs; "/p:BuildProjectReferences=false" |]

let testNames =
stdOutput
Expand Down Expand Up @@ -1493,8 +1507,11 @@ module Interactions =
let runnableTests = TestItem.runnableFromArray projectRunRequest.Tests

let projectPath = projectRunRequest.ProjectPath
let! _ = DotnetCli.restore projectPath
let! buildStatus = MSBuild.invokeMSBuildWithCancel projectPath "Build" _ct
let basePathArgs = Project.getOutputPaths ()

let! _ = DotnetCli.restore projectPath basePathArgs

let! buildStatus = MSBuild.invokeMSBuildWithCancel projectPath "Build" _ct basePathArgs

if buildStatus.Code <> Some 0 then
TestRun.showError testRun "Project build failed" runnableTests
Expand Down Expand Up @@ -1568,7 +1585,11 @@ module Interactions =
promise {
let projectPath = project.Project
logger.Info($"Building {projectPath}")
let! processExit = MSBuild.invokeMSBuildWithCancel projectPath "Build" cancellationToken

let basePathArgs = Project.getOutputPaths ()


let! processExit = MSBuild.invokeMSBuildWithCancel projectPath "Build" cancellationToken basePathArgs
return (project, processExit)
})

Expand Down