msbuild
msbuild copied to clipboard
dotnet
Update to net7.0
Fixes #
Context
Changes Made
Testing
Notes
This will be chipped away at over time.
Hey @rainersigwald, @BenVillalobos, this will be needed for source-build in 7.0. I was testing this locally before I saw this PR, I am hitting the same issues as the CI here. Is there anything I can do to help move this along?
@lbussell We're in early stages of working on this (Read: I'm poking at this when I have time 🙃). If you'd like to poke at it and offer some commits we can cherry-pick onto this branch, you're more than welcome! Feel free to PM me directly on this
I pushed the branch I was working on to here. The only thing I tried differently was adding nowarn for some of the SYSLIB errors, which gave more errors: msbuild.txt
@lbussell I cherry-picked your commit to resolve the future issues you were seeing. The "This type is not part of the declared API" wasn't showing up for me. Typically when those come up (which they shouldn't here?) you just do the VS suggested action of adding them to some Public.Shipped.API.txt file. Good news is I got a green build after that! Pushing it up to see if tests fail.
As for the issue that was nowarn'd around, it looks like the deprecations to AssemblyName are going to require us to rethink our use of AssemblyName. It looks like we use AssemblyName to gather information about the generated binaries. https://github.com/dotnet/runtime/issues/59061#issuecomment-922392861 suggests looking into the System.Reflection.Metadata namespace for gathering assembly info. Even if we could do that, it looks like we shouldn't use AssemblyName anymore, or we may need to create our own type.
The main culprits that need this type look like ResolveAssemblyReference and BinaryTranslator.
Based on my (very) brief investigation, I don't think working around AssemblyName is a reasonable thing to get volunteer work on, though we appreciate the offer! 😭
Realistically, I suspect we'll be able to dig in more early next week.
Thinking about this some. Our build defines the netstandard/netcore public shipped API txt files to be the same. Now that we're moving to net7.0, technically none of the API's in "netstandard" have been shipped yet because we haven't shipped these API's in net7.0.
Not 100% sure this is what's happening under the hood, but I'm going to clear out public api's shipped txt under netstandard and have VS fix it back into unshipped.
That shouldn't be the problem but heck if I know what the actual problem is (doesn't repro on my machine either).
Specifically removing the API's causing errors and having VS resolve them doesn't fix it.
Marking functions as public "because the analyzer didn't see it" doesn't resolve it either
@sharwell any guidance here? API's are clearly public, but the analyzers are complaining about them. Maybe I'm missing something
Preventing the warning from AssemblyName's ProcessorArchitecture being deprecated will make our caches not deserialize properly. Solution: delete the cache once (or just build again, since it should just be a warning), and it'll work the second time.
@Forgind 's commits for future reference: https://github.com/forgind/msbuild/tree/net7.0-here-we-go
@sharwell any guidance here? API's are clearly public, but the analyzers are complaining about them. Maybe I'm missing something
@jaredpar I just realized @sharlwell is oof, any ideas?
You're running afoul of a bug in the new ref safety rules. It is upset about this constructor:
https://github.com/dotnet/msbuild/blob/3a92cefaf053c9377037d0af0c67736867cc9711/src/StringTools/InternableString.cs#L36
It believes the ctor may capture the parameter by ref. This is actually a bug in the current toolset. The RC SDK will compile this without error. Short term you can mark the reference as scoped ref and it will fix the problem.
@AraHaan /shrug. We could change that, but there's still the task of obtaining the processor architecture, which we're exploring how to do in a reasonable way: https://github.com/dotnet/runtime/issues/74040
@AraHaan /shrug. We could change that, but there's still the task of obtaining the processor architecture, which we're exploring how to do in a reasonable way: dotnet/runtime#74040
So basically getting the architecture of the assembly itself + the .NET Host (to compare and prevent loading the wrong architecture in the host architecture)?
I've simplified things a bit for future generations: Most places that needed a manual change from 6 to 7 was replaced with a single property. I also added a comment including relevant places to modify when updating to net8.0+
On the comment will you need to update those files in the PR as well (the ones mentioned on it).
@AraHaan Yep!
Edit: Unfortunately they're scripts and it's nontrivial to have an msbuild property flow into those.
Hi @BenVillalobos, what's the status of this? Can it be merged soon? Thanks!
This is not currently planned, because it breaks an SDK invariant (SDK tools should target LTS .NET). We have a meeting coming up to discuss a path forward with various stakeholders.
@rainersigwald Please consider adding the source-build team to the meeting if it's appropriate (myself, @crummel, @MichaelSimons). We were hoping to get this change in for RC2.
This is not currently planned, because it breaks an SDK invariant (SDK tools should target LTS .NET). We have a meeting coming up to discuss a path forward with various stakeholders.
I think it should be on for the .NET 7 SDK only, but yeah I would be ok with it targeting LTS for LTS sdk builds.
@BenVillalobos @rainersigwald @jaredpar, from the discussion we had in the meeting, is the plan now to merge this to flush out any potential issues going forwards?
Internal official build for good measure https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=6700270&view=results
Potential workaround for our roslyn analyzer issue: https://github.com/dotnet/msbuild/issues/7903
@lbussell I suspect sourcebuild will also want us on Arcade 7 and I'm working on that migration now.