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 as a dependency of the runtime #77029

Closed
wants to merge 3 commits into from

Conversation

tlakollo
Copy link
Contributor

Add cecil as a dependency of the runtime
Enables the subscription

@tlakollo tlakollo requested a review from sbomer October 13, 2022 21:59
@tlakollo tlakollo self-assigned this Oct 13, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

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

I think you might need to run darc update-dependencies --source-repo https://github.com/dotnet/cecil to get the version info in here.

Also, can you put the info in Versions.props into the super large <PropertyGroup> at the bottom next to the <StaticCsVersion> item with a comment above it stating which repo it's from (the extra line ends up significantly reducing merge conflicts)?

@ghost
Copy link

ghost commented Oct 13, 2022

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details

Add cecil as a dependency of the runtime
Enables the subscription

Author: tlakollo
Assignees: tlakollo
Labels:

area-Infrastructure

Milestone: -

@kasperk81
Copy link
Contributor

why is cecil needed when there is a managed typesystem with dependency analysis available for production use?

@jkoritzinsky
Copy link
Member

There’s plans to move the linker to the runtime repo: #75278

The linker uses Cecil, so a Cecil dependency is needed until the linker moves to the managed type system, if that ever happens.

Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

LGTM

@ericstj
Copy link
Member

ericstj commented Oct 19, 2022

Why add a dependency that is not consumed anywhere? All this does is create a property that's not used and can get out of sync, and churns the repo in PRs to update that property. Can you wait to make this change until you consume that property?

@tlakollo tlakollo added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 19, 2022
@tlakollo
Copy link
Contributor Author

Why add a dependency that is not consumed anywhere? All this does is create a property that's not used and can get out of sync, and churns the repo in PRs to update that property. Can you wait to make this change until you consume that property?

Yes, so far Im prototyping how this would work with consuming the cecil package in #77149. Not sure if just mark it as NO-MERGE or just close this PR

@joperezr
Copy link
Member

It's usually best to keep the PR as a draft PR and to have the no-review and no-merge labels if we are not intending to merge this. Otherwise, people might come to the PR, see that it is approved and green, and might just merge it.

@tlakollo
Copy link
Contributor Author

Closing this since #78077 got merged

@tlakollo tlakollo closed this Nov 17, 2022
@tlakollo tlakollo deleted the AddCecilDependency branch November 17, 2022 14:11
@ghost ghost locked as resolved and limited conversation to collaborators Dec 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants