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 BuildOM forward compatibility #10394

Merged
merged 2 commits into from
Jul 18, 2024
Merged

Add BuildOM forward compatibility #10394

merged 2 commits into from
Jul 18, 2024

Conversation

JanKrivanek
Copy link
Member

@JanKrivanek JanKrivanek commented Jul 17, 2024

Fixes #10349

Context

The OM unification work #10172 introduced backwards compatible unified OM (code compiled against old MSBuild binaries can be run against new MSBuild binaries), but it was not forward compatible (code compiled against new MSBuild binaries could not be run against old MSBuild binaries). This change is bridging such gap.

Changes Made

Adding 'proxy' properties for those that have been pulled to base classes - so that the compiled code is not trying to reference base classes, that do not exist in the old version of MSBuild code.

Testing

Manual testing of x-compatibility of the OM.

  1. Add a binary (e.g. console app) with code that is using the OM and it's properties - e.g.:
            Microsoft.Build.Graph.GraphBuildRequestData data =
                new(
                    projectFullPath: "a/b/c",
                    globalProperties: new Dictionary<string, string>() { { "a", "b"} },
                    targetsToBuild: new List<string>() { "t1", "t2"},
                    hostServices: null);

            Console.WriteLine("target names:" + data.TargetNames.FirstOrDefault());
  1. Build and run the code (just sanity check) - on the new version of MSBuild and old version of MSBuild
  2. Copy the created binary from the old version of MSBuild over to the output of new MSBuild (testing of backwards compatibility) and run
  3. Copy the created binary from the new version of MSBuild over to the output of old MSBuild (testing of forward compatibility) and run

Notes

Big thanks to @rainersigwald and @dfederm for the detailed help and ideas during troubleshooting the problem

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

Can you add some tests that use reflection to make sure these things continue to exist? The comment is good but a test expresses intent even more clearly IMO.

@JanKrivanek
Copy link
Member Author

Can you add some tests that use reflection to make sure these things continue to exist? The comment is good but a test expresses intent even more clearly IMO.

Tests added - populated with pre-refactoring set of properties and methods (for simplicity it doesn't verify method signatures, but simple signature change would be catched by ApiCompat. It'd need to be a case of multiple overloads, while some are moved to base - and in the pre-refactoring OM there were no overloads of the provided public methods)

@JanKrivanek JanKrivanek enabled auto-merge (squash) July 17, 2024 17:53
@JanKrivanek JanKrivanek merged commit 9045cf7 into main Jul 18, 2024
10 checks passed
@JanKrivanek JanKrivanek deleted the bugfix/buildom-fwd-compat branch July 18, 2024 07:33
@JanKrivanek JanKrivanek mentioned this pull request Jul 18, 2024
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Type load exception for BuildRequestDataBase thrown upon getting cache result
4 participants