-
Notifications
You must be signed in to change notification settings - Fork 5.2k
New Configuration System #1787
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
New Configuration System #1787
Conversation
Is the trailing semicolon required? It looks odd. |
|
I love the cleanup. What will be the effect on the experience when I open projects in VS? I think you mentioned some things will improve, some will not? Clearly making our projects look more like regular projects is a good thing 😄 |
eric has mentioned some of the pros here in the issue https://github.com/dotnet/corefx/issues/41512 |
…inalTargetFramework
...libraries/System.Reflection.Emit.ILGeneration/src/System.Reflection.Emit.ILGeneration.csproj
Show resolved
Hide resolved
src/libraries/System.Reflection.Emit.Lightweight/src/System.Reflection.Emit.Lightweight.csproj
Show resolved
Hide resolved
| @@ -7,6 +7,9 @@ | |||
| <CoverageReportDir>$(ArtifactsDir)coverage</CoverageReportDir> | |||
| <SerializeProjects>true</SerializeProjects> | |||
| </PropertyGroup> | |||
| <PropertyGroup> | |||
| <TargetFramework>netcoreapp5.0</TargetFramework> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this required for P2P to work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SDK requires TargetFramework to set TargetFrameworkIdentifier as this property cant be empty.
There is no use for it other than that
|
I went through all changes and left comments. Decide for yourself (based on the impact) to either address it in this PR or a follow-up one. |
Thanks Viktor for the review. I will try to address as many as possible while i am waiting/fixing the ci |
TargetFrameworkSuffix refers to OS part specified in the targetFramework string. we set TargetsWindows\Linux on the basis of this property. OSGroup referes to the OS we are building the product for. Earlier we were using the OSGroup for TargetFrameworkSuffix as well. Earlier we were setting this property using buildConfiguration and configuration that we were passing from the outer build to the innerbuild. |
|
Hello @Anipik! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
When you say the "OS part of TargetFramework", doesn't that imply the OS we are targeting? So how is that different from OSGroup which also describe as the "OS we are building for"? |
|
"OS we are building for" could be Linux, but a project could just define |
|
I see that makes sense. Then BuildOS would really have been a better name, wouldn't it? OSGroup doesn't mean much at all. |
|
Yeah. I pushed back towards introducing |
|
Another thought: what if you made BuildTargetFramework be the thing with the OS in it? Then derive OSGroup and TargetFramework from that? Would that be sufficient? Or do we still need to understand the parts of the original BuildTargetFramework at places lower in the stack that will have different values for OSGroup and TargetFramework as appropriate for their projects? |
BuildOS could have been better, but another point @safern raised was that it might confuse developers who already use OSGroup to build stuff. So we wanted to keep the behavior same as well. |
|
Yeah, but now that I see how it behaves fully I was mostly confused, it might make sense to bring BuildOS back, or do something like @ericstj suggested if it meets our needs. |
|
And another thought. If we start using real RIDs instead of our made up ones we could use the property that defines the Packaging RID for this purpose |

Things Done
Things left
This is for the first review.
Fixes #31052