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

Vs insertion Sbom generation (Roslyn,fSharp) #9629

Merged
merged 11 commits into from
Jun 15, 2022

Conversation

epananth
Copy link
Member

@epananth epananth commented Jun 9, 2022

To double check:

Repos like Roslyn, vs-code-coverage and Fsharp have multiple VSIXs per VS component. Previously when we created the feature for sbom generation, we assumed that we have one VSIX per VS component but then we hit some corner case, since there are many vsix per component, the build used to error saying that the sbom has already been generated for the VS component. Here in this fix, I have made sure that no matter how many vsix the vs component has, it will unpack all the vsix and generate sbom for them individually and it will be linked to the vs manifest file.

@epananth epananth self-assigned this Jun 9, 2022
@epananth epananth requested a review from mmitche June 14, 2022 00:19
@epananth epananth marked this pull request as ready for review June 14, 2022 00:19
@epananth
Copy link
Member Author

epananth commented Jun 14, 2022

@epananth epananth requested a review from riarenas June 14, 2022 21:34
@epananth epananth mentioned this pull request Jun 15, 2022
2 tasks
Copy link
Member

@mmitche mmitche left a comment

Choose a reason for hiding this comment

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

Looks generally good. Can you add a description to the PR indicating what the current problem is, what the changes are to solve it?

We require that all VSIXes included in a single VS insertion component have the same version.
This version will be set to ManifestBuildVersion.
-->
<Error Text="Visual Studio component '$(ComponentName)' contains multiple VSIX files with different versions: @(_VsixVersionNoDuplicates->'%(VsixFileName) (version %(Identity))', ', ')"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the error message should mention that all versions need to match for sbom generation? if I got this error I wouldn't understand what's wrong about having different versions in my components, which doesn't seem that strange (if a new vsix is added it doesn't seem weird to me that it would have a different version than existing ones)

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually for sbom generation we are just using the Vsix version. This error msg is specifically for VS manifest generation. So I am going to add something like

"Cannot generate VS manifest because Visual Studio Component '$(ComponentName)' contains multiple VSIX files with different versions: @(_VsixVersionNoDuplicates->'%(VsixFileName) (version %(Identity))', ', ')" 

This way we will know that vs manifest is not generated cos of version error.

@epananth
Copy link
Member Author

adding an example to make this part clearer..

@epananth epananth requested review from mmitche and riarenas June 15, 2022 20:00
@epananth epananth added auto-merge Automatically merge PR once CI passes. labels Jun 15, 2022
@epananth epananth added auto-merge Automatically merge PR once CI passes. labels Jun 15, 2022
@ghost
Copy link

ghost commented Jun 15, 2022

Hello @epananth!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Copy link
Member

@riarenas riarenas left a comment

Choose a reason for hiding this comment

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

Not a big deal but I'd like my comment to be addressed (even with a "not possible")

https://github.com/dotnet/arcade/pull/9629/files#r898372524

@epananth epananth removed the auto-merge Automatically merge PR once CI passes. label Jun 15, 2022
@epananth epananth merged commit 8d38ec4 into dotnet:main Jun 15, 2022
@epananth
Copy link
Member Author

@riarenas I was trying to split that once, like you suggested. But I was not successful, so I asked @alexperovich and he mentioned I should probably move that a different task and is probably not worth it. So I don't think I will be able to do that.

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.

4 participants