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 cecil dependency to ILLink package #82630

Merged
merged 2 commits into from
Feb 24, 2023
Merged

Add cecil dependency to ILLink package #82630

merged 2 commits into from
Feb 24, 2023

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented Feb 24, 2023

Fixes #82597 by ensuring the Microsoft.DotNet.Cecil dependency shows up in the nuspec. Fixes #82611.

The ref project changes aren't strictly necessary since that project isn't used for packaging, but I removed PrivateAssets there for consistency. I removed Publish there because it should never be published in a package from that project.

I think this wasn't caught in #82407 because the package validation doesn't run as part of the clr.tools subset - we should fix that.

@sbomer sbomer requested a review from marek-safar as a code owner February 24, 2023 17:56
@ghost ghost added the linkable-framework Issues associated with delivering a linker friendly framework label Feb 24, 2023
@ghost ghost assigned sbomer Feb 24, 2023
@ghost
Copy link

ghost commented Feb 24, 2023

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #82597 by ensuring the Microsoft.DotNet.Cecil dependency shows up in the nuspec. Fixes #82611.

The ref project changes aren't strictly necessary since that project isn't used for packaging, but I removed PrivateAssets there for consistency. I removed Publish there because it should never be published in a package from that project.

Author: sbomer
Assignees: -
Labels:

linkable-framework

Milestone: -

@stephentoub
Copy link
Member

I assume this will also fix #82611?

@sbomer
Copy link
Member Author

sbomer commented Feb 24, 2023

Yes, that's the intention. Do you know if there's a way to trigger the libraries build legs in this PR? I checked the .nuspec locally but ideally we would run package validation in this change.

@ViktorHofer
Copy link
Member

I checked the .nuspec locally but ideally we would run package validation in this change.

I don't think there's an easy way todo with our current infrastructure setup. Did you consider changing the evaluate path triggers to make sure that the libraries allconfigurations leg (which invokes packagevalidation) runs when src/tools/illink changes?

@sbomer
Copy link
Member Author

sbomer commented Feb 24, 2023

Did you consider changing the evaluate path triggers to make sure that the libraries allconfigurations leg (which invokes packagevalidation) runs when src/tools/illink changes?

I considered this, but I think we wanted to run only the illink validation legs when developing illink. Would it be feasible to run package validation in the clr.tools subset instead?

@ViktorHofer
Copy link
Member

I considered this, but I think we wanted to run only the illink validation legs when developing illink. Would it be feasible to run package validation in the clr.tools subset instead?

Just to be clear, two "package validation" features exist:

  1. Package Validation which is part of APICompat (shipping tool). This runs whenever we do a dotnet pack or a dotnet build + GeneratePackageOnBuild=true. We only do this in the Libraries Windows allconfigurations leg because doing a pack in every leg would end up generating duplicated package assets which will make the official build's Publish step fail (because it doesn't know which one to pick).
  2. testPackages.proj. This is the infrastructure that does dependency closure verification and the component that emitted the error. I don't know of an easy way to enable this in the tools subset.

Just enabling the Libraries Windows allconfigurations would enable both components and just adds a single leg to the pipeline. I would advise doing that, especially as we want to run the live built linker at some point in the libraries partition anyway.

To run package validation (dependency closure verification)
@sbomer
Copy link
Member Author

sbomer commented Feb 24, 2023

That makes sense, thanks a lot for the explanation!

@agocke
Copy link
Member

agocke commented Feb 24, 2023

Looks like the libraries build passed, which is all we really need for this change. Merging.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
linkable-framework Issues associated with delivering a linker friendly framework
Projects
None yet
4 participants