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

Revert "Update BuildHost STJ to 8.0.4" #75528

Merged
merged 1 commit into from
Oct 16, 2024
Merged

Conversation

tmat
Copy link
Member

@tmat tmat commented Oct 16, 2024

This reverts commit 6f65bfc.

System.Text.Json.dll was eliminated from BuildHost by #73393 but then re-added back by flow from internal branch.

@tmat tmat requested a review from a team as a code owner October 16, 2024 15:20
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 16, 2024
@tmat tmat merged commit bd973bf into dotnet:release/dev17.12 Oct 16, 2024
23 of 25 checks passed
@tmat tmat deleted the RevertSTJ branch October 16, 2024 17:00
@arunchndr
Copy link
Member

@jaredpar and @jasonmalinowski fyi

@ericstj
Copy link
Member

ericstj commented Oct 16, 2024

I pulled this down and see the assets file still contains STJ - perhaps it's coming transitively?

@ericstj
Copy link
Member

ericstj commented Oct 16, 2024

It looks like Microsoft.Build brings it in, then CPM will update it.

      "Microsoft.Build/17.3.4": {
        "type": "package",
        "dependencies": {
          "Microsoft.Build.Framework": "17.3.4",
          "Microsoft.IO.Redist": "6.0.0",
          "Microsoft.NET.StringTools": "17.3.4",
          "System.Collections.Immutable": "6.0.0",
          "System.Configuration.ConfigurationManager": "6.0.0",
          "System.Memory": "4.5.5",
          "System.Reflection.MetadataLoadContext": "6.0.0",
          "System.Text.Json": "6.0.0",
          "System.Threading.Tasks.Dataflow": "6.0.0"
        },

@ericstj
Copy link
Member

ericstj commented Oct 16, 2024

The MSBuild reference is ExcludeAssets="Runtime" which means this fix did remove the dll from the net472 output which is good, but the package is still mentioned in the deps file, which we've been trying to avoid.

      "System.Text.Json/8.0.4": {
        "dependencies": {
          "System.Runtime.CompilerServices.Unsafe": "6.0.0",
          "System.Text.Encodings.Web": "8.0.0"
        }
      },

@marcpopMSFT
Copy link
Member

@ericstj do we need roslyn to update the STJ in the directory.packages.props now to resolve the deps.json entry coming from MSBuild? Do we need a 17.12 msbuild first to be able to fix this (which we can't do until we have VS updated?)?

@ericstj
Copy link
Member

ericstj commented Oct 17, 2024

You could either:

  1. Update STJ to 8.0.5 only for net6.0 and exclude it -- and sign up to keep updating it every time there's an update in 8.0. 👎 (means ongoing servicing coordination between 8 and 9 where we waterfall 8.0 changes through roslyn then back into 9.0)
  2. Make the SDK responsible for rewriting the deps file like it does for other tools. That would erase the mention of the stale dependencies.
  3. Use a different mechanism for excluding MSBuild's package dependency closure. I have a slightly hacky way for doing this -- https://github.com/dotnet/runtime/blob/43813ac73242fa78c463d456bf755e3a6622b5d7/eng/PackageDownloadAndReference.targets#L5

@tmat
Copy link
Member Author

tmat commented Oct 17, 2024

@ericstj
Re [2] - can you point me to the targets in SDK that do so for other tools?

@ericstj
Copy link
Member

ericstj commented Oct 17, 2024

Have a look at https://github.com/search?q=repo%3Adotnet%2Fsdk%20RemoveAssetFromDepsPackages&type=code I think this is what does it.
TBC I don't own this, but back when we were looking at servicing @rainersigwald and I discovered that the SDK is rewriting the deps file for tools which allows MSBuild to stay out of servicing flow. I think @marcpopMSFT owns this stuff, but it may be old and untouched for years.

@ericstj
Copy link
Member

ericstj commented Oct 17, 2024

I pulled down a binlog and found it's actually this project that's doing it: https://github.com/dotnet/sdk/blob/ea38ebafb21205f84fe0536a3255c030afaf2f79/src/Layout/redist/redist.csproj#L26

It looks like it actually produces a deps file for the entire SDK "folder" (which will reference the latest packages) then it removes instances of the tools from that deps file, then it copies that deps file to each tool.deps.json instead of using the one provided by the tool:
https://github.com/dotnet/sdk/blob/ce0abad026fcae9d670952aed61c915a1028355a/src/Layout/redist/targets/GenerateLayout.targets#L231-L252

You can see here -- search for $target GenerateCliRuntimeConfigurationFiles

@tmat
Copy link
Member Author

tmat commented Oct 17, 2024

Oh, I think the redist stuff is just for setting up dogfooding env and testing. I don't think these are for production.

@marcpopMSFT
Copy link
Member

Yeah, redist.csproj prepares the SDK repo assets to be bundled up. redist-installer.csproj does the bundling up (we basically used to flow a nupkg output of redist.csproj into the installer repo and use that in the installer build). We have plans to eventually combine the two redist projects post repo merge.

And per Eric, I don't think we've touched this logic much in a few years since we did some perf optimizations.

@ericstj
Copy link
Member

ericstj commented Oct 18, 2024

Also -- not sure if you caught the distinction -- the SDK doesn't remove libraries - it just removes mention of some files. The way it's actually making "live version" deps files for tools is by generating an entirely new deps file that's produced by referencing a superset of packages and then replacing the tool's deps file with that one renamed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants