-
Notifications
You must be signed in to change notification settings - Fork 520
Conversation
Also needed to update msbuild version past 16.10 because of a bug that meant that the default targets of projects couldn't be built.
This resolves #1187 |
var solutionBatch = new Dictionary<string, List<BuildRequest>>(); // store the list of promises | ||
var projectBatch = new Dictionary<string, List<BuildRequest>>(); | ||
// TODO: quiet time... maybe wait both...? | ||
await Task.Delay(100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there a delay here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the idea of having a quiet time where we wait to see if more files will be saved, then schedule the builds at once... it would be better to have it waiting 100ms (or a more sensible duration) after the last WaitToReadAsync(). Unsure what would happen if it was removed; might still need to yield to let duplicate project build requests enter the queue?
@james-allan-lloyd Sorry for it taking so long to be able to get to this. I hope you don't take offense to the changes I've made; feel free to suggest others. I'd appreciate if you could take the latest code for a spin and see if it continues to work in the way you expect. |
@ravipal This is another one to review, when you have a chance. (Comment added since the PR's been open a while and the original notification would've been long ago.) |
No worries, I'll get back to you soon on it. |
Moving forward with this PR in order to make progress toward 0.11; we'll deal with any fallout, if any, as separate PRs. |
Hi guys, I only just now got to checking out the changes and they work for me (4 or so projects depending on a 2 shared libraries). One small caveat; when using the solution build, it doesn't like nested targets (I'm using dotnet msbuild version 17.0.0+c9eb9dd64). This appears to be the default now when doing dotnet sln add... I worked around it by commenting out the To be clear, my day to day work project looks a bit like this:
I need to invoke msbuild like this to build a targeted project:
|
{ | ||
logger.LogInformation("Build Watcher: Building {Targets} of solution {SolutionPath}...", targets, solutionPath); | ||
|
||
var buildResult = await ProcessUtil.RunAsync("dotnet", $"msbuild {solutionPath} -target:{targets}", throwOnError: false, workingDirectory: workingDirectory, cancellationToken: cancellationToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for #1438, this code is not replacing "special characters" with an _
in the targets.
https://docs.microsoft.com/en-us/visualstudio/msbuild/how-to-build-specific-targets-in-solutions-by-using-msbuild-exe?view=vs-2022
(I believe that documentation has a bit of a bug in itself, that last "in the specified target name" should be "in the specified project name" to not be confused with the target name portion of -targets argument.)
Use a queue for build requests in
--watch
mode so that duplicate builds can be removed, and optionally use a solution to build multiple projects to avoid dependencies being built simultaneously and causing file locking races.Example config (albeit the simplest approach):
note-1: I'm new to C# but experienced in other languages. It's very possible that I am not doing things in a canonical C# way.
note-2: to work around a bug in msbuild when using the
-target
commandline arg, I had to upgrade msbuild past version 16.10 (I'm using 16.11.0.36601 specifically).Resolves #580
Resolves #1187