Skip to content

Conversation

@jimmylewis
Copy link
Contributor

Fixes two issues in the recent changes to the targets file in the Build package:

  • The path to the build task assembly was incorrectly updated since we now build the project for net8.0 and net481. Also, it was updated to use an MSBuild variable that exists in this repo, but the targets file runs in the context of the consumer (which wouldn't have it defined). So the path was returned to a hard-coded value. This will need to be maintained as target frameworks get updated (or we need a better way to generate the targets file).
  • The Build package was passing the dependency to System.Runtime.Loader transitively. This was causing warnings with .NET 9's stricter security advisory due to the runtime dependencies (System.Private.Uri specifically). This should not be a transitive dependency for consumers, so setting it to PrivateAssets fixes the issue.

Resolves #770

During the upgrade to target .NET 8, any references to prior targeted
frameworks were removed.  In this case, the hardcoded framework path was
replaced with $(NetFxTFM) which is defined in the libman build, but not
in the consumer's build (where this .targets file is evaluated).  It
should not have been replaced here.

I've reset it back to hard-coded values, and updated the frameworks
currently targeted by the project (net481 and net8.0).
This dependency shouldn't be passed transitively to consumers.  It is also causing warnings with .NET 9's stricter security advisory due to the runtime dependencies.
@phil-allen-msft
Copy link
Contributor

Is there a reasonable test to prevent recurrence?

@jimmylewis
Copy link
Contributor Author

Not at the moment. Unit testing doesn't catch the "reality" of running within msbuild (which could be either netfx or netX.0). We need some integration tests that actually use the package to build, and we need to test both runtime environments. I can try to come up with a separate PR to add such a test later, I think it would be good to have (similarly, I'd like to add some "real" integration tests for the CLI exe as well, rather than the bootstrapping unit tests we do now).

I manually tested these changes by using dotnet build (for net8.0 runtime) and building within VS (which, AFAICT from the errors I was getting previously, uses the netfx msbuild). Before the fix, each gave me different errors, and after the fix, both work fine.

@phil-allen-msft phil-allen-msft merged commit 4e15779 into aspnet:main Dec 9, 2024
2 checks passed
@jimmylewis jimmylewis deleted the fixBuildTargets branch March 1, 2025 09:25
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.

Microsoft.Web.LibraryManager.Build starting to show security warnings

2 participants