-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Target net7.0 in additional projects #27012
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
Target net7.0 in additional projects #27012
Conversation
| <!-- Intentionally pinned. This feature is supported in projects targeting 6.0 or newer.--> | ||
| <TargetFramework>net6.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.
My biggest concern is here: I couldn't find a good reason why this says its pinned. Is there any extra validation I can do to make sure this doesn't break anything?
If absolutely necessary, we can set the TFM to net7.0 for source-build and net6.0;net7.0 otherwise.
a06e47f to
e19ccb0
Compare
ViktorHofer
left a comment
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.
I thought about this change a bit more after our offline discussion and I think we don't want to upgrade the tfm for APICompat projects from net6.0 to net7.0 as they also produce packages which should continue to be consume-able by customers that target net6.0.
I would assume that we only bump the tfm when building via source build.
|
@ViktorHofer, so I should amend this PR to conditionally set the TFM to net7.0 for source-build? Who are the customers of APICompat? If you're concerned about breaking customers, do you think that anyone would be affected if APICompat only targeted net7.0 in source-build (i.e. RHEL and Ubuntu users)? |
|
Nope. We don't ship packages produced in source build publicly and components inside the stack shouldn't be using any of the ApiCompat packages as ApiCompat itself is already part of the SDK. Right. It should be fine to only build net7.0 in source build. Alternatively we could always target net7.0 as well but for source build remove the other target frameworks. |
| <PropertyGroup> | ||
| <TargetFrameworks>net7.0;net472</TargetFrameworks> | ||
| <TargetFrameworks>net6.0;net472</TargetFrameworks> | ||
| <TargetFrameworks Condition="'$(DotNetBuildFromSource)' == 'true'">net7.0;net472</TargetFrameworks> |
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.
Why do we build net472 when source building?
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.
We try to keep source-build as close to the product build as possible. If we don't lose anything by dropping net472 here, I'm fine with removing it though.
|
@tmat Do you know what might be going wrong with the dotnet watch tests here? Might I be getting a path wrong somewhere? |
|
No, the tests are flaky. Fix is on the way: #27700 |
* Remove timeouts from tests * Fix
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@tmat Can you take another look? I cherry picked your changes and the newest test failure is a timeout: The original error was unrelated to timeouts: I'm trying to figure out why only these dotnet watch tests are unable to find System.Runtime, I suspect it is because of the paths I changed. |
|
The same issue, see the dump: |
7bacfab to
fc4a200
Compare
|
@ViktorHofer, @ericstj, @tmat, can you all review this? We wouldn't like to carry this as a source-build patch beyond RC2 as it can cause conflicts and block CI. |
tmat
left a comment
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.
I can't think of why the Delta Applier needs to be net6.0 either. It is loaded into the target application by dotnet-watch, but the app should run on the same .NET version as dotnet-watch does so even if the app targets net6.0 the Delta Applier should be able to call net7.0 .NET API.
ViktorHofer
left a comment
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.
The APICompat changes LGTM.
These net6.0 target frameworks are still bringing in prebuilts in source-build. They need to be changed before we can ship net7.0 without prebuilts.