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

Fix .NET Native AOT warnings in Protobuf reflection #11128

Closed

Conversation

JamesNK
Copy link
Contributor

@JamesNK JamesNK commented Dec 2, 2022

The fixes from #10978. This PR can be merged without updating the SDK. Will allow people to use Protobuf + AOT sooner rather than later.

When the repo is updated to .NET 7 or .NET 8, the original PR can be rebased on latest to add AOT analysis and provide some AOT smoke tests.

cc @jskeet

@jskeet
Copy link
Contributor

jskeet commented Dec 2, 2022

Thanks. I'm on vacation for the weekend but will look on Monday.

Copy link
Contributor

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

Thanks for this! Will run it through Kokoro and get it merged. Hopefully we can get the SDK update and tests in reasonably soon afterwards.

@jskeet
Copy link
Contributor

jskeet commented Dec 5, 2022

(The actual merge will be slightly delayed as we're sorting out internal mirroring for the C# codebase; I'll talk with the team about whether we should force-merge this one.)

@jskeet jskeet added mergeable:force-allow Break-glass label to allow merging PRs directly without Copybara. and removed mergeable:force-allow Break-glass label to allow merging PRs directly without Copybara. labels Dec 5, 2022
@jskeet
Copy link
Contributor

jskeet commented Dec 14, 2022

Just FYI, I haven't forgotten about this. We have some internal source control changes that I'm trying to get done before Christmas. They may slip into the New Year - but I definitely want to get this in.

@mkruskal-google mkruskal-google requested review from deannagarcia and removed request for deannagarcia January 10, 2023 18:16
@JamesNK JamesNK force-pushed the jamesnk/enable-aot-analysis-2 branch from 6ebecc7 to 51ed1bd Compare January 17, 2023 22:57
@JamesNK JamesNK requested a review from a team as a code owner January 17, 2023 22:57
@JamesNK JamesNK requested review from jtattermusch and removed request for a team January 17, 2023 22:57
@JamesNK JamesNK mentioned this pull request Jan 17, 2023
3 tasks
@JamesNK
Copy link
Contributor Author

JamesNK commented Jan 17, 2023

Rebased.

@JamesNK
Copy link
Contributor Author

JamesNK commented Jan 20, 2023

Nice!

@jskeet Are you able to give a rough estimate of when this will be available in a release?

cc @jtattermusch

@jskeet
Copy link
Contributor

jskeet commented Jan 20, 2023

@JamesNK: I'm expecting it "pretty soon" if everything goes smoothly. I'd rather not be more precise than that and put the Protobuf team in a corner :)

@jtattermusch
Copy link
Contributor

@JamesNK
I might be missing context, but AFAICT, protobuf has only done releases from the 21.x branch for the past several months and I don't know of a plan to create a new release branch anytime soon (that might be just my ignorance). Since the timeline for the next release branch is unclear, I'd say it's a safer bet to proactively create a PR that backports to 21.x branch, so that the fix appears in the next patch release of Google.Protobuf nuget (which is going to 3.21.x).
Especially since you mentioned that this change is somewhat time sensitive.

@jskeet
Copy link
Contributor

jskeet commented Jan 20, 2023

I'd say it's a safer bet to proactively create a PR that backports to 21.x branch, so that the fix appears in the next patch release of Google.Protobuf nuget (which is going to 3.21.x).

Please hold off on this for now - sorry about the mixed messages; too many teams involved!

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

Successfully merging this pull request may close these issues.

6 participants