Skip to content
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

Add ILVerify from the corert repo #35038

Merged
merged 9 commits into from
Jun 2, 2020
Merged

Add ILVerify from the corert repo #35038

merged 9 commits into from
Jun 2, 2020

Conversation

agocke
Copy link
Member

@agocke agocke commented Apr 16, 2020

No significant source modifications were made, only moving the code and any missing
dependencies.

Contributes to #13827

No significant source modifications were made, only moving the code and any missing
dependencies.
@@ -10,6 +10,7 @@
<ProjectReference Condition="'$(BuildManagedTools)' == 'true'" Include="tools/runincontext/runincontext.csproj" />
<ProjectReference Condition="'$(BuildManagedTools)' == 'true'" Include="tools/r2rdump/R2RDump.csproj" />
<ProjectReference Condition="'$(BuildManagedTools)' == 'true'" Include="tools/ReadyToRun.SuperIlc/ReadyToRun.SuperIlc.csproj" />
<ProjectReference Condition="'$(BuildManagedTools)' == 'true'" Include="tools/ILVerification/ILVerification.csproj" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this really have to land in coreclr. I'd love to use this even with coreclr build disabled

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

90+% of sources for ILVerify are shared with the CoreCLR AOT compiler. It gets unnatural to have the sources in one place and the project including the sources in different place, just to make it fit better with top level build script.

Do any of these options look reasonable to you?

  • Use it from package if you do not want to build CoreCLR?
  • Invoke the build of this .csproj file from Mono build by default?
  • Add a new subset for this so that you can build just the tool, without the rest of CoreCLR?

This is touching on larger problem that the current coreclr/mono directory structure is unfriendly to sharing. It is reasonable expect that we will want to share more and more in future. What should the directory structure friendly to sharing between coreclr/mono look like?

Copy link
Contributor

@marek-safar marek-safar Apr 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do any of these options look reasonable to you?

I'd like to use dotnet tool ilverifier regardless of what runtime I'm building.

This is touching on larger problem that the current coreclr/mono directory structure is unfriendly to sharing.

I think this is not necessarily about runtime sharing but about having a way to build shared tools for dotnet/runtime but I'm not opposed to solving both of these at once.

@agocke
Copy link
Member Author

agocke commented Apr 16, 2020

Thanks all! I created this PR because I'd really like to use ILVerify in Roslyn, but I admit I don't really understand how the layout or build for CoreCLR works. I made this a draft mostly so I could see how CI works, because it's not clear to me how things necessarily build or how the tests run.

My overall goal would be to get this published as a NuGet package that Roslyn could pull in. Any advice on that front? Is CoreCLR still the right place to put this? Or should it go under libraries? Those shared files would be the main sticking point to moving it.

Also, I can't figure out how to get the tests set up. It looks like XUnit isn't commonly used and I can't figure out where a good entry point would be for the tests.

@jkotas
Copy link
Member

jkotas commented Apr 16, 2020

NuGet package
tests

If this is build under CoreCLR:

If we were to decide that it is better to build this under libraries (see the other comment):

@agocke
Copy link
Member Author

agocke commented May 27, 2020

@jkotas Thanks! I haven't had time to look into the issues here. If you want to take over, that's fine by me.

@jkotas
Copy link
Member

jkotas commented May 28, 2020

Happy to help. This should be running tests and publishing ILVerification library package now. The .pkgproj project to build the package is heavy-handed, but I could not find a better to do that under src/coreclr.

The remaining part is to add ilverify global tool package. It may be better done as a separate follow up change.

@jkotas jkotas marked this pull request as ready for review May 28, 2020 03:28
@jkotas
Copy link
Member

jkotas commented May 28, 2020

@jkoritzinsky @ViktorHofer Could you please take a look at Microsoft.ILVerification.pkgproj in this change? Is there a way to do it in a better way in the current build structure?

@jkotas
Copy link
Member

jkotas commented May 29, 2020

ilverify global tool package.

It was not that hard to do, so I have done that as well.

C:\test>dotnet tool install dotnet-ilverify --version 5.0.0-dev
You can invoke the tool from this directory using the following commands: 'dotnet tool run ilverify' or 'dotnet ilverify'.
Tool 'dotnet-ilverify' (version '5.0.0-dev') was successfully installed. Entry is added to the manifest file C:\test\.config\dotnet-tools.json.

C:\test>dotnet ilverify hello.dll -r "c:\Program Files\dotnet\shared\Microsoft.NETCore.App\2.1.12\*.dll"
All Classes and Methods in C:\test\hello.dll Verified.

@agocke
Copy link
Member Author

agocke commented Jun 1, 2020

LGTM

@jkotas jkotas merged commit 9a22600 into dotnet:master Jun 2, 2020
@agocke agocke deleted the add-ilverify branch June 2, 2020 00:14
@jashook
Copy link
Contributor

jashook commented Jun 3, 2020

This change unfortunately has broken our outerloop test build. I have not entirely figured out why. But this is blocking testing (#36862) and merging #37274

@jashook
Copy link
Contributor

jashook commented Jun 3, 2020

/cc @dotnet/runtime-infrastructure

@jashook
Copy link
Contributor

jashook commented Jun 3, 2020

https://dev.azure.com/dnceng/public/_build?definitionId=655&_a=summary for a list of jobs. First failure of this type is after the merge :(

@jkotas
Copy link
Member

jkotas commented Jun 3, 2020

I will take a look

@CarolEidt
Copy link
Contributor

Thanks @jashook!

@jashook
Copy link
Contributor

jashook commented Jun 4, 2020

After a quick look I do not believe we support referencing managed tools projects currently like this from our test build: https://github.com/dotnet/runtime/pull/35038/files?file-filters%5B%5D=.csproj&file-filters%5B%5D=.ilproj#diff-f7c5c72ccd9aaf2be3cf9840805daccdR23.

It seems to require us to have restored the managed tools which may have not have happened during the test build.

@jashook
Copy link
Contributor

jashook commented Jun 4, 2020

In CI we would not have done this for example.

@jashook
Copy link
Contributor

jashook commented Jun 4, 2020

/cc @jkoritzinsky

@jashook
Copy link
Contributor

jashook commented Jun 4, 2020

Also just a build of clr+libs would not build our managed tools I believe @jkoritzinsky to correct me if I am wrong.

@jashook
Copy link
Contributor

jashook commented Jun 4, 2020

./build.sh --subset clr+libs -c release --runtimeConfiguration checked
./dotnet.sh msbuild src/coreclr/tests/src/ilverify/ILVerification.Tests.csproj 

For a quick repro

@jashook
Copy link
Contributor

jashook commented Jun 4, 2020

Trying to verify if this is correct with:

./build.sh --subset clr.tools+clr+libs -c release --runtimeConfiguration checked
./dotnet.sh msbuild src/coreclr/tests/src/ilverify/ILVerification.Tests.csproj /p:Configuration=Checked

@jkotas
Copy link
Member

jkotas commented Jun 4, 2020

This is failing for me with Assets file 'D:\runtime\src\coreclr\tests\src\Common\test_dependencies\obj\project.assets.json' not found that is a different error than the error hit by the CI.

@jashook
Copy link
Contributor

jashook commented Jun 4, 2020

Still failing

@jkotas
Copy link
Member

jkotas commented Jun 4, 2020

A lot of other coreclr tests will fail like this if you try to build without restore.

@safern
Copy link
Member

safern commented Jun 4, 2020

This is failing for me with Assets file 'D:\runtime\src\coreclr\tests\src\Common\test_dependencies\obj\project.assets.json' not found that is a different error than the error hit by the CI.

Instead of doing

./dotnet.sh msbuild src/coreclr/tests/src/ilverify/ILVerification.Tests.csproj /p:Configuration=Checked

You can run:

./dotnet.sh build src/coreclr/tests/src/ilverify/ILVerification.Tests.csproj /p:Configuration=Checked

That will restore and build.

@jkotas
Copy link
Member

jkotas commented Jun 4, 2020

This should be fixed by #37388

@nattthebear
Copy link

@jkotas Was this tool actually published? I can't seem to find the package on nuget.org.

PS > dotnet tool install --global dotnet-ilverify --version 5.0.0-dev
error NU1101: Unable to find package dotnet-ilverify. No packages exist with this id in source(s): C:\Program Files\dotnet\sdk\NuGetFallbackFolder, nuget.org
The tool package could not be restored.

@akoeplinger
Copy link
Member

akoeplinger commented Jun 30, 2020

@nattthebear you'll need to add the dotnet5 nuget feed until everything is pushed to nuget.org:

dotnet tool install --global dotnet-ilverify --version "5.0.0-*" --add-source "https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet5/nuget/v3/index.json"

@nattthebear
Copy link

@akoeplinger Thank you very much! Working here now 👍

@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants