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

Move MSBuild initialization from MSBuildTestBase to MSBuildAssemblyResolver #277

Merged
merged 2 commits into from
Mar 14, 2024

Conversation

MattKotsenas
Copy link
Contributor

The MSBuild location / initialization code was only available in the MSBuildTestBase abstract class for unit tests.

Using a base class for this purpose can create multiple inheritance problems when combined with other testing tools. To allow more flexibility in how the assembly resolution is done, I moved the code to MSBuildAssemblyResolver.Register(), and forward the call from MSBuildTestBase for backwards compatibility.

I wrapped registration in a Lazy to prevent registration from happening multiple times. However, I don't think that's technically necessary, so I can remove that if desired.

I also updated the README section on resolving MSBuild. I'm not an SDK resolution expert, but I believe the old option 2 was insufficient, so I removed it and added a new "preferred" method of using a Module initializer if possible (which is also morally equivalent).

@MattKotsenas
Copy link
Contributor Author

Separately, is MSBuildAssemblyResolver still needed, or would it make sense to deprecate it in favor of the MSBuildLocator package? I didn't see a rationale for the private implementation in the commit history, but:

  1. I'm not an SDK resolution expert
  2. That code is at least 6 years old, so rationale may be stale

If you prefer, I'm also happy to add a dependency on MSBuildLocation and mark MSBuildAssemblyResolver as obsolete instead.

@MattKotsenas
Copy link
Contributor Author

One more item: Though net462 doesn't support it natively, since the project targets a new enough version of C#, we could implement a module initializer ourselves so consumers don't need to do any registration / initialization at all. That however assumes:

  1. There's a sane default to register
  2. Custom registration could override if desired

I believe these things are both true, but I didn't want to jump to that conclusion without double-checking.

README.md Show resolved Hide resolved
@jeffkl
Copy link
Owner

jeffkl commented Mar 12, 2024

Separately, is MSBuildAssemblyResolver still needed, or would it make sense to deprecate it in favor of the MSBuildLocator package? I didn't see a rationale for the private implementation in the commit history, but:

  1. I'm not an SDK resolution expert
  2. That code is at least 6 years old, so rationale may be stale

If you prefer, I'm also happy to add a dependency on MSBuildLocation and mark MSBuildAssemblyResolver as obsolete instead.

I worked on MSBuildLocator and don't really like it. Its yet another assembly to do a simple thing (resolve MSBuild). It also doesn't use the defaults I think are best so I just implemented one here. If the one here is out of date or has an issue, we can reassess but for now I think its fine to have a custom one.

@jeffkl jeffkl merged commit 64930d4 into jeffkl:main Mar 14, 2024
3 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.

None yet

2 participants