Make sure Memory and Unsafe are packaged w/o RID#16244
Conversation
| netstandard1.0; | ||
| netstandard; | ||
| netcoreapp-Windows_NT; | ||
| netcoreapp-Unix; |
There was a problem hiding this comment.
This must keep OS-specific configs since it references the core assembly which is different per-os.
The workaround being used by our build system to only build these packages on Windows was leaking into the packaging. To fix that I overrode the packaging RID for Unsafe, since it was the only thing that actually needed the filtering. Then to fix Memory I added a reference assembly for unsafe and built against that instead.
ericstj
force-pushed
the
refactorMemoryUnsafeConfigs
branch
from
0f421ca to
fbafeb4
Compare
| <PropertyGroup> | ||
| <BuildConfigurations> | ||
| netstandard1.0; | ||
| netstandard; |
There was a problem hiding this comment.
Which version out of these two will end up in the netcoreapp flat layout?
It should be the netstandard1.0 one so that it does not have crazy dependencies.
There was a problem hiding this comment.
the netstandard one will.
What do you mean by "crazy" dependencies? Currently they're both building against System.Runtime, but the "netstandard" one should eventually build against netstandard.dll (once ilasm is fixed). Both System.Runtime.dll and netstandard.dll are part of netcoreapp so I don't see either of them as "crazy".
There was a problem hiding this comment.
S.R.CS.Unsafe is very low (depends on System.Runtime only) and it is being used to implement other netcoreapp assemblies that are very low (System.Memory, likely more in future).
netstandard.dll is very high. If S.R.CS.Unsafe bundled into netcoreapp depends on netstandard.dll, we will end up with dependency cycle in netcoreapp implementation.
Do we have something in place to catch implementation dependency cycles in the new build? If not, we should add it. Dependency cycles are nightmare to untangle once they sneak in.
There was a problem hiding this comment.
Yes, we do catch dependency cycles. The VerifyClosure task runs against MS.NETCore.App and catches exactly the type of cycle you suggest.
If we do run into a situation where a low-level assembly needs to use this we can cross-compile for NETCoreApp and have it reference either System.Runtime, or even System.Private.Corelib. If you'd prefer to undo the factoring I did to allow this project to specify the core-assembly I can do that too. I left it in because it seemed like the "right thing to do" to build a netstandard version of this library, and because it uncovered a bug in ilasm.
Currently this assembly isn't even in the shared framework FYI.
There was a problem hiding this comment.
catches exactly the type of cycle you suggest
Awesome! Thanks
If you'd prefer to undo the factoring
This factoring is good because of it gives us flexibility. My concern was only about what kind of build we get for netcoreapp.
we can cross-compile for NETCoreApp and have it reference either System.Runtime, or even System.Private.Corelib
Agree. We will want to do that as we try to do #15768 again. cc @ahsonkhan @mellinoe
@ericstj Have you opened a bug on it? |
|
@ericstj are you planning to add these guys to the shared framework as a separate change? |
I have not. Since we were still using the desktop ilasm here I didn't want to create noise for you. I think we need to test with the coreclr ilasm before filing a bug in coreclr. Let's let dotnet/buildtools#1301 track that.
No. Its unclear to me if these need to be in the shared framework. My understanding based on discussion with @ahsonkhan is that System.Memory timeline may not align with nca2.0 timeline but I could be wrong. Not sure what folks want for Unsafe but none of my existing closure-based rules will bring it in. |
|
I think Span itself should typeforward but you want the System.Memory span extensions to remain in package (so for example the extensions have access to System.Numerics.Vector etc) |
…afeConfigs Make sure Memory and Unsafe are packaged w/o RID Commit migrated from dotnet/corefx@b31ed13
7 participants
The workaround being used by our build system to only build these
packages on Windows was leaking into the packaging.
To fix that I overrode the packaging RID for Unsafe, since it was the
only thing that actually needed the filtering.
Then to fix Memory I added a reference assembly for unsafe and built
against that instead.
Eventually we'll be able to remove the filtering from unsafe but having
a reference assembly for this is general goodness.
I also tried to tackle cross-compiling Unsafe for netstandard.dll and hit
what appears to be a bug in
ilasm. It didn't like me claiming thatSystem.Object was in netstandard, and instead tried to insist it was in
mscorlib and added a reference to that to the output assembly. I could
change System.Object to some other type (eg: System.Attribute) and it
was fine. @jkotas I think this is one of those hard-coding-the core assembly
bugs. We should see if it's fixed by using a newer ilasm (@mellinoe is
working on that), if not we should hunt it down and fix it in coreclr.
/cc @mellinoe @ahsonkhan @weshaggard