Expose Assembly.Location via public contract#3861
Conversation
There was a problem hiding this comment.
This + the System.Runtime.Loader dependency just caused these tests to not be runnable on .NET Native. We should consider splitting these out so we can still use these reflection tests on more of our platforms.
There was a problem hiding this comment.
Pragmatically, all 3 tests are actually dependent on not being run on .NET Native at the moment. The .NET Native test will just be Assert.NotNull(ThisAssembly().Location); Assert.Empty(ThisAssembly.Location()). However, we don't currently share enough infrastructure to express that directly here in corefx repo.
There was a problem hiding this comment.
All I want to be careful about is that when we start running these test on .NET Native that we don't loose all the reflection tests because of these new tests. If we think that might happen perhaps we should factor some of these out into another test project.
There was a problem hiding this comment.
Fair point. Split out the tests in to a separate project.
nguerrera
force-pushed
the
assembly-location
branch
4 times, most recently
from
d18c877 to
df68985
Compare
|
Does it also need to be flipped to public in https://github.com/dotnet/coreclr/blob/master/src/mscorlib/model.xml ? |
|
@jkotas Hmm, I imagined it was already there as tests are passing and I see the property and its public getter in ildasm of coreclr mscorlib. I am now trying to understand what is making that happen without the entry where I'd expect it to be in model.xml... |
nguerrera
force-pushed
the
assembly-location
branch
from
df68985 to
d7555ae
Compare
There was a problem hiding this comment.
Nice. We should probably update other .sln fils to include the ref projects as well.
|
golf clap 👏 |
There was a problem hiding this comment.
Can we somehow build TinyAssembly.dll as part of the build process rather than checking in the binary?
|
One question, otherwise LGTM. |
nguerrera
added
the
api-needs-work
|
@stephentoub I'm also using it in #3896 to get a known-guid -- i.e. depending on it not changing. (C#'s new deterministic feature would help with that eventually.) The file is ~1.5KB uncompressed and ~500B compressed. The contents are deliberately as minimal as possible and I don't think it will change much if at all. S.R.Loader also has source for a small assembly, but also checks in the dll to embed the resource. (That can probably be fixed by using ProjectReference with OutputItemType=EmbeddedResource, mind you.) We're embedding as resource in both cases to get around corerun putting everything in sight on the TPA list. |
There was a problem hiding this comment.
I assume you want to use a package reference once these changes get published? If so please file a tracking issue.
|
LGTM |
|
I submitted dotnet/coreclr#1783 to be explicit in the model file. |
terrajobst
added
the
api-approved
|
@weshaggard, @KrzysztofCwalina and myself just took a look. Approved 👍 |
nguerrera
force-pushed
the
assembly-location
branch
from
d7555ae to
c57eba5
Compare
|
Added missing copyright headers and caught up to master in rebase. No other changes since LGTMs. I will merge when tests pass. |
|
@dotnet-bot test this please (https://github.com/dotnet/corefx/issues/3915) |
Expose Assembly.Location via public contract
Expose Assembly.Location via public contract Commit migrated from dotnet/corefx@126241e
9 participants
After some discussion, it seems we should just expose Assembly.Location after all. It already does not guarantee a path (string.Empty for load from stream/byte[], NotSupportedException for dynamic assembly). On .NET Native, we can simply return String.Empty.
This has been requested many times over: #2221, dotnet/coreclr#1367 (+ lots of requests offline).
Note that there are still scenarios not covered such as getting source path in DNX when compiled on the fly, but I think we can continue to discuss that separately. I believe those scenarios are better served by a separate API rather than an Assembly.Location override.
cc @weshaggard @jkotas