-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Run 'AddImplicitDefineConstants' on 'BeforeCompile' #44695
Run 'AddImplicitDefineConstants' on 'BeforeCompile' #44695
Conversation
e894826
to
ae2b63a
Compare
ae2b63a
to
4cd69e8
Compare
@rainersigwald CI is all green (but I don't have permissions), can we merge this? 😄 Also, do you think we could backport this to .NET 9? Should I fill out the usual servicing form? Thank you! |
I'd need @rainersigwald to weigh in on how risky moving this is as we typically don't take this sort of change for 9. 9.0.2xx is open but this is borderline. We'd need high confidence of no break to consider for 1xx. |
If it helps, 9.0.2xx would also be fine (I assume that'd go out somewhere in January?). My concern was mostly about not being able to get the fix until all the way to November 2025 with .NET 10. So even just 9.0.2xx would be better than that 😅 |
I think 2xx sounds reasonable for this @marcpopMSFT. |
Sounds reasonable. @Sergio0694 , feel free to backport to 9.0.2xx so as not to have to wait a full year. |
Awesome, will do that. Thank you both! 🙂 EDIT: opened #44938. |
Fixes #43908
This PR changes "AddImplicitDefineConstants" to use "BeforeCompile" as trigger, instead of "CoreCompile".