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 back a line that was lost in dotnet/installer migration #41537

Merged
merged 1 commit into from
Jun 12, 2024

Conversation

MichalStrehovsky
Copy link
Member

dotnet/installer main has had this line since April. This repo doesn't have it.

Cc @dotnet/ilc-contrib

dotnet/installer main has had this line since April. This repo doesn't have it.
@ViktorHofer ViktorHofer requested a review from MiYanni June 12, 2024 07:54
@ViktorHofer
Copy link
Member

ViktorHofer commented Jun 12, 2024

@MichalStrehovsky should we add tests that use the win-x86 ILCompiler RID to avoid potential regressions in the future?

@MichalStrehovsky
Copy link
Member Author

@MichalStrehovsky should we add tests that use the win-x86 ILCompiler RID to avoid potential regressions in the future?

We have a partial coverage of this list. Full coverage is not possible because we can't cover the freebsd RID (that is there so that the community can build for this if they supply a package) and I'm not sure if we can test things like MUSL since they need a crossrootfs to target.

I think testing for win-x86 would just fall out from these

public class GivenThatWeWantToPublishAnAotApp : SdkTest
but maybe we never run tests with the win-x86 SDK?

But the general answer is that we rely on code reviews as a gate to prevent bugs in this list.

@ViktorHofer ViktorHofer merged commit a738d2a into main Jun 12, 2024
36 checks passed
@ViktorHofer ViktorHofer deleted the MichalStrehovsky-patch-1 branch June 12, 2024 09:38
@MiYanni
Copy link
Member

MiYanni commented Jun 12, 2024

Yeah, it looks like it just fell through the cracks when reintegrating the history in the Phase 1 merge PR: https://github.com/dotnet/sdk/pull/38804/files#diff-394534bbcc1d9d8fdf376e8caca2def8ae61560e7f50862ad8106e07072d6ebeR401-R414
The likely reason is because the indentation in this file was fixed on this side (SDK repo) but not on the installer side. Also, I could have just missed this value since this particular include line has a lot of entries.

Here's the original PR in the installer repo for reference: dotnet/installer#19166

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants