Add "cache add" functionality to project caching

[dfederm]

This change add "cache add" functionality to project caching.

Today project caching exposes a hook to plugins for "I'm about to build this thing, do you want to take over instead?" and that's it. This change adds a few more hooks which report file accesses and processes (via Detours) and also for when a project finishes building. This allows a plugin to collect the file accesses and add entries to the cache for future replayability.

I strongly recommend reviewing each commit as opposed to the whole PR as a whole. I (hopefully) broke it down into reasonably reviewable chunks.

CC @AndyGerlicher @DmitriyShepelev

[Forgind]

Partial review (next commit to review is c738443)

With your explanation from this morning, your code is easier to review than I would've expected—I appreciate it.

My primary concern is making sure you don't worsen the experience of building without this flag set, something I intend to look into properly after I've looked at the rest of the PR when I give it a more wholistic look.

[Forgind]

Partial review (next commit to review is c738443)

With your explanation from this morning, your code is easier to review than I would've expected—I appreciate it.

My primary concern is making sure you don't worsen the experience of building without this flag set, something I intend to look into properly after I've looked at the rest of the PR when I give it a more wholistic look.

[Forgind]

Looks pretty good! I didn't see anything too concerning as I went through, but I haven't signed off yet because it's too big to absorb everything at once, and I haven't properly looked through all your comments.

[Forgind]

Looks pretty good! I didn't see anything too concerning as I went through, but I haven't signed off yet because it's too big to absorb everything at once, and I haven't properly looked through all your comments.

[Forgind]

Unrelated, but why is GitHub putting my comment above and below my inline comments? I don't think it's done that on other PRs.

[JanKrivanek]

Mostly looks fine.
I have added couple minor comments. My main concerns currently:

• The change feels that it deserves some additional tests
• Public nuget feed added
• Possibly avoidable locking in FileAccessManager
• Is there a chance that the additional dependencies might be missing by VS after insrtion? I have very vague knowledge here - so not sure how to best verify this other then experimental insertion. But it'd be uncovered after merging this PR anyway - so not a big concern

[Forgind]

Is there a chance that the additional dependencies might be missing by VS after insrtion? I have very vague knowledge here - so not sure how to best verify this other then experimental insertion. But it'd be uncovered after merging this PR anyway - so not a big concern

I think it's worth a serious look and an exp/ branch. A couple years ago, I added a reference to a new System package, and it prevented us from inserting until we reverted it a couple months later. That was Bad.

https://github.com/dotnet/msbuild/pull/8726/files#r1190408764

[dfederm]

Is there a chance that the additional dependencies might be missing by VS after insrtion? I have very vague knowledge here - so not sure how to best verify this other then experimental insertion. But it'd be uncovered after merging this PR anyway - so not a big concern

I think it's worth a serious look and an exp/ branch. A couple years ago, I added a reference to a new System package, and it prevented us from inserting until we reverted it a couple months later. That was Bad.

https://github.com/dotnet/msbuild/pull/8726/files#r1190408764

I tried this but am hitting the same errors I see in the PR gate. I'll need help resolving those issues as I have not attempted to introduce new dependencies to MSBuild before.

[Forgind]

I made an exp/ branch for this PR (at its current version). Build result here. It looks like we'll have to add BuildXL to the baseline package (list?) used by source build for this to get past the official build stage. Then we can check whether it passes RPS.

Anyone know how to do that?

[JanKrivanek]

I made an exp/ branch for this PR (at its current version). Build result here. It looks like we'll have to add BuildXL to the baseline package (list?) used by source build for this to get past the official build stage. Then we can check whether it passes RPS.

Anyone know how to do that?

As for source build - they should be added to source-build-reference-packages - instructions
Then we need to make sure we reference appropriate version of source-build-reference-packages - that should happen automagicaly once the issues with #8893 get resolved, as I created darc subscribtion.
Those are basically analogous steps to what I'm going through with #8903.

As for adding the binaries to VS and then requesting and getting exception for RPS/DDRit - there I unfortunately have no experience

[rainersigwald]

Overall looks good to me. I have a few small comments and a few larger themes:

• How should we think about testing here? Is it possible to build a mock so that the MSBuild codepaths can be exercised in this repo? If not how can you let us know ASAP if we break your plugin?
• I want an opt-in API to report actual reads from e.g. roslyn compiler server; is that extending this or a separate thing?
• Can we get at least a sketch of docs for the functionality and plugin requirements?
  • one thing I really want is the timing stuff: there's the secret marker report thing, but how does that sync with the scheduler's knowledge of what nodes are doing? is there a race between _scheduler!.GetExecutingRequestByNode and the marker report?

[dfederm]

I want an opt-in API to report actual reads from e.g. roslyn compiler server; is that extending this or a separate thing?

Yup, see EngineServices.ReportFileAccess. That's how a task would self-report its accesses.

[dfederm]

Aggregating remaining unaddressed feedback up to this point so I don't forget it (I find GitHub threading a bit hard to track over multiple iterations):

• Audit tests to ensure proper coverage
  • E2E tests would be really nice, although this might be out of scope for now
• Improve docs to properly describe the state of the feature
• Razor compiler server... figure out if we need to allow this to breakaway as well

Reviewers, please let me know if I missed any of your feedback

[dfederm]

Spoke with @rainersigwald offline.

So that this PR can make the 17.8 deadline, we agreed to defer the UTs (and ideally e2e tests) and docs to a separate PR which I'm promising to deliver soon.

wrt Razor compiler server, I'm hesitant to allow it to break away since that would lose the detours information even when not running in server mode (rzc.exe is used for both scenarios). We will likely need to revisit this at some point though...