Skip to content

C#: Delete the CIL extractor. #15794

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Mar 12, 2024
Merged

C#: Delete the CIL extractor. #15794

merged 5 commits into from
Mar 12, 2024

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Mar 4, 2024 •
edited
Loading

In this PR the CIL extractor is deleted, including all CIL related tests and the extractor option of enabling CIL extraction.
Commit by commit review is encouraged.

@github-actions github-actions bot added the C# label Mar 4, 2024
@michaelnebel michaelnebel changed the title C#: Delete the CIL implementation. C#: Delete the CIL extractor. Mar 5, 2024
@michaelnebel michaelnebel force-pushed the csharp/removecil branch 2 times, most recently from 961c193 to d46a643 Compare March 5, 2024 11:19
@michaelnebel michaelnebel mentioned this pull request Mar 6, 2024
Merged
@michaelnebel
Copy link
Contributor Author

DCA experiment: https://github.com/github/codeql-dca-main/issues/19722

@michaelnebel michaelnebel marked this pull request as ready for review March 8, 2024 07:34
@michaelnebel michaelnebel requested a review from a team as a code owner March 8, 2024 07:34
@michaelnebel
Copy link
Contributor Author

DCA looks good!

tamasvajk
tamasvajk reviewed Mar 11, 2024
View reviewed changes
Copy link
Contributor

@tamasvajk tamasvajk left a comment

Choose a reason for hiding this comment

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

Looks okay to me. The tests are failing though.

hvitved
hvitved requested changes Mar 11, 2024
View reviewed changes
Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

Looks like changes are needed in the internal repo as well.

@michaelnebel
Copy link
Contributor Author

The internal PR mentions this PR, but it looks like it drowned in all the other mentions :-)
Tests are passing on the internal PR - I will add you both as reviewers on that one as well.

@tamasvajk
Copy link
Contributor

@hvitved Do you know why the standalone tests are failing?

hvitved
hvitved requested changes Mar 12, 2024
View reviewed changes
Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

We need to remove "cil" from
https://github.com/github/codeql/blob/main/csharp/extractor/Semmle.Util/CommandLineOptions.cs#L43

@hvitved
Copy link
Contributor

hvitved commented Mar 12, 2024

@hvitved Do you know why the standalone tests are failing?

Now I do :-) See my comment above.

@michaelnebel
Copy link
Contributor Author

@hvitved Do you know why the standalone tests are failing?

Now I do :-) See my comment above.

OMG - that was important. The standalone extractor would crash in case someone had set the environment variable 😱
Thank you both!!!

Searched both codeql and semmlecode again for more cil related stuff, but didn't find any.
Is there any workflow that sets the CIL environment variable that I have overlooked?

@michaelnebel michaelnebel merged commit 5085121 into github:main Mar 12, 2024
@michaelnebel michaelnebel deleted the csharp/removecil branch March 12, 2024 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hvitved hvitved hvitved approved these changes

+1 more reviewer

@tamasvajk tamasvajk tamasvajk left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
C# documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@michaelnebel @tamasvajk @hvitved