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

Solves the problem of getting project MSBuild information #34574

Merged
merged 2 commits into from
Jan 17, 2025

Conversation

paulomorgado
Copy link
Contributor

@paulomorgado paulomorgado commented Aug 30, 2024

This PR replaces the GetEFProjectMetadata target in EntityFrameworkCore.targets with Evaluate items and properties and display results of targets.

Fixes #23853
Fixes #30725

  • I've read the guidelines for contributing and seen the walkthrough
  • I've posted a comment on an issue with a detailed description of how I am planning to contribute and got approval from a member of the team
  • The code builds and tests pass locally (also verified by our automated build checks)
  • Commit messages follow this format:
        Summary of the changes
        - Detail 1
        - Detail 2

        Fixes #bugnumber
  • Tests for the changes have been added (for bug fixes / features)
  • Code follows the same patterns and style as existing code in this repo

- Refactor MSBuild integration to remove the need for EntityFrameworkCore.targets and GetEFProjectMetadata target.

Fixes dotnet#23853
@AndriySvyryd AndriySvyryd self-assigned this Aug 30, 2024
@paulomorgado
Copy link
Contributor Author

Hi @AndriySvyryd. Do you need anything from me on this PR?

@AndriySvyryd
Copy link
Member

Ideally the PR should include tests, but we currently don't have the infrastructure for this. Once #33136 is done I'll take a look at the PR. It was submitted too late for 9.0 anyway, so there's no rush.

@paulomorgado
Copy link
Contributor Author

I looked for the tests and couldn't find them.

You can reach out to someone at MSBuild to check this.

This can easily go to .NET 8.0, because the SDK supports it. I've been using this for a while.

@bricelam
Copy link
Contributor

Fixes #32113

@bricelam
Copy link
Contributor

Err wait, not quite. Remove --msbuildprojectextensionspath now, since it should no longer be needed.

@paulomorgado
Copy link
Contributor Author

Err wait, not quite. Remove --msbuildprojectextensionspath now, since it should no longer be needed.

Was it, for sure, used only for this purpose? If it was, we should remove it.

@bricelam
Copy link
Contributor

Yes. It just told us where to copy the MSBuild file

@paulomorgado
Copy link
Contributor Author

Err wait, not quite. Remove --msbuildprojectextensionspath now, since it should no longer be needed.

Removed!

@paulomorgado
Copy link
Contributor Author

Hi @AndriySvyryd,

Is anything missing from my part here?

@AndriySvyryd
Copy link
Member

@paulomorgado No, it's on my radar.

@AndriySvyryd AndriySvyryd merged commit fbce1d5 into dotnet:main Jan 17, 2025
7 checks passed
@AndriySvyryd
Copy link
Member

Thanks for your contribution!

@KennethHoff
Copy link

I'm assuming asking for a backport on this is a no-go? 😅

@paulomorgado
Copy link
Contributor Author

I'm assuming this will be ported to all supported versions.

Some customers need LTS support.

@KennethHoff
Copy link

Generally speaking they don't backport "features" like this though

@paulomorgado
Copy link
Contributor Author

This is safe to port to 8.

@bricelam
Copy link
Contributor

Some customers need LTS support.

EF 10 will be LTS... 😏

@AndriySvyryd
Copy link
Member

This is safe to port to 8.

Not with our definition. It changes the implementation significantly and there are no tests covering it.

@paulomorgado
Copy link
Contributor Author

This is safe to port to 8.

Not with our definition. It changes the implementation significantly and there are no tests covering it.

I can port all the tests covering this in 9 to 8.

@AndriySvyryd
Copy link
Member

AndriySvyryd commented Jan 17, 2025

I can port all the tests covering this in 9 to 8.

The are no tests. This has always been an issue. We only test ef.dll, but not the shims that use it.

#33136, #32616

@paulomorgado
Copy link
Contributor Author

I can port all the tests covering this in 9 to 8.

The are no tests. This has always been an issue. We only test ef.dll, but not the shims that use it.

#33136, #32616

I know!

If it's good enough for 9, it's good enough for 8.

Or are you thinking on releasing this only for 10?

@AndriySvyryd
Copy link
Member

Or are you thinking on releasing this only for 10?

Yes. Due to the above, this is a risky change that we only do early in the release to give early adopters enough time to exercise it before it's released to the general audience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants