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

[cdac] Implement a JSON contract reader #100966

Merged
merged 18 commits into from
Apr 19, 2024
Merged

Conversation

lambdageek
Copy link
Member

@lambdageek lambdageek commented Apr 12, 2024

Contributes to #99298

Implement a parser for the "compact" JSON contract descriptor format specified in data_contracts.md

The data model is a WIP - it's likely we will want something a bit more abstract (and less mutable).

This is not wired up to consume a real contract descriptor from target process memory at the moment. It's only exercised by unit tests for now.

TODO:

  • hook up the unit tests to some testing infrastructure - either src/libraries or src/tests or to something else

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 12, 2024
@lambdageek lambdageek added area-Diagnostics-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 12, 2024
lambdageek and others added 2 commits April 12, 2024 15:47
Co-authored-by: Aaron Robinson <arobins@microsoft.com>
@lambdageek
Copy link
Member Author

By the way, the unit tests for cdacreader aren't hooked up to any CI infrastructure at the moment. I'm not sure if we should include them into src/tests or src/libraries or directly into some yaml pipeline. Open to suggestions.

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
@jkoritzinsky
Copy link
Member

For hooking up the unit tests, I think hooking it up in the same way as the ILCompiler and ILLink unit tests will be the easiest way.

That way it won't take on additional dependencies or complexity introduced by the other test harnesses.

@lambdageek
Copy link
Member Author

For hooking up the unit tests, I think hooking it up in the same way as the ILCompiler and ILLink unit tests will be the easiest way.

@jkoritzinsky just so I'm clear: I would need to add a new lane in the yaml file, right?

@jkoritzinsky
Copy link
Member

You could add a subset and add that subset to the CLR Tools Tests job's build args (and update the path triggers). Then it will run as part of that job. No need for an additional job.

we incorrectly allowed `[number]` as a field descriptor
conversion. that's not allowed.  removed it.
Dont' use libraries test infrastructure.  Just normal arcade xunit support.
@lambdageek
Copy link
Member Author

Added a tools.cdacreadertests subset - added it to the CLR_Tools_Tests test job

Copy link
Member

@elinor-fung elinor-fung left a comment

Choose a reason for hiding this comment

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

Are there docs anywhere about tools tests (adding, running, etc) that we should update?

lambdageek and others added 2 commits April 17, 2024 10:50
Co-authored-by: Elinor Fung <elfung@microsoft.com>
@lambdageek
Copy link
Member Author

Are there docs anywhere about tools tests (adding, running, etc) that we should update?

Added a new PR with docs: #101186

@lambdageek
Copy link
Member Author

lambdageek commented Apr 17, 2024

.dotnet/sdk/9.0.100-preview.3.24204.13/Sdks/Microsoft.NET.Sdk/targets/Microsoft.PackageDependencyResolution.targets(266,5): error NETSDK1047: (NETCORE_ENGINEERING_TELEMETRY=Build) Assets file '/__w/1/s/artifacts/obj/cdacreader/project.assets.json' doesn't have a target for 'net9.0/linux-x64'. Ensure that restore has run and that you have included 'net9.0' in the TargetFrameworks for your project. You may also need to include 'linux-x64' in your project's RuntimeIdentifiers.

there's a relevant failure on the "CLR_Tools_Tests" lane that is similar to what we previously saw with the src/native/managed infrastructure - it's something about the right properties not flowing down to the Restore target, I think.

Copy link
Member

@elinor-fung elinor-fung left a comment

Choose a reason for hiding this comment

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

Looks good to me - assuming whatever is going on with the CLR_Tools_Tests leg is straightforward to address.

…roperties

Since we set Configuration and RuntimeIdentifier, if we don't pass the
same AdditionalProperties in all ProjectReferences, we bad
project.assets.json files
@lambdageek
Copy link
Member Author

Ah. The issue was that we now have two <ProjectReference Include="cdacreader.csproj"> items. One in compile-native.proj and the other in the unit test. But they had distinct AdditionalProperties metadata, and the one in compile-native.proj set Configuration and RuntimeIdentifier, whereas the test didn't. as a result the restore created a project.assets.json that was only correct for one reference and not the other. the fix is to set the same AdditionalProperties on both references.

We set RuntimeIdentifier=$(OutputRID) so that in the case of cross-arch build NativeAOT will target the target architecture. but I wonder if there's a better way to do that than by messing with ProjectReference - which is apparently kind of fragile.

@lambdageek lambdageek merged commit 43b22a8 into dotnet:main Apr 19, 2024
149 of 152 checks passed
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
Implement a parser for the "compact" JSON contract descriptor format specified in [data_contracts.md](https://github.com/dotnet/runtime/blob/main/docs/design/datacontracts/data_descriptor.md)

The data model is a WIP - it's likely we will want something a bit more abstract (and less mutable).

This is not wired up to consume a real contract descriptor from target process memory at the moment.  It's only exercised by unit tests for now.

---

* compact descriptor format json parser

* suggestions from reviews; remove FieldDescriptor wrong conversion

   we incorrectly allowed `[number]` as a field descriptor conversion. that's not allowed.  removed it.

* Make test project like the nativeoat+linker tests

   Dont' use libraries test infrastructure.  Just normal arcade xunit support.

* add tools.cdacreadertests subset; add to CLR_Tools_Tests test leg

* no duplicate fields/sizes in types

* Make all cdacreader.csproj ProjectReferences use the same AdditionalProperties

   Since we set Configuration and RuntimeIdentifier, if we don't pass the same AdditionalProperties in all ProjectReferences, we bad project.assets.json files

* Don't share the native compilation AdditionalProperties

---------

Co-authored-by: Aaron Robinson <arobins@microsoft.com>
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
Co-authored-by: Elinor Fung <elfung@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators May 19, 2024
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.

None yet

5 participants