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

[ci] Fix codeql build #5739

Merged
merged 6 commits into from
Sep 18, 2024
Merged

[ci] Fix codeql build #5739

merged 6 commits into from
Sep 18, 2024

Conversation

radical
Copy link
Member

@radical radical commented Sep 16, 2024

Avoid doing test specific tasks when not building to run tests.

Fixes #5738 .

Microsoft Reviewers: Open in CodeFlow

Avoid doing test specific tasks when not building to run tests.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-codeflow for labeling automated codeflow. intentionally a different color! label Sep 16, 2024
@radical
Copy link
Member Author

radical commented Sep 16, 2024

@radical radical added area-engineering-systems and removed area-codeflow for labeling automated codeflow. intentionally a different color! labels Sep 16, 2024
@@ -35,7 +35,7 @@

<!-- Used for running one helix job per test class -->
<Target Name="_ExtractTestClassNames"
Condition="'$(ExtractTestClassNamesForHelix)' == 'true'"
Condition="'$(ExtractTestClassNamesForHelix)' == 'true' and '$(ArchiveTests)' == 'true'"
Copy link
Member

Choose a reason for hiding this comment

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

Why are we building anything in the tests directory in this build configuration? If we don't want to do "test things", do we need to build the test assemblies?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good question. AFAICS, even if we have -p:Tests=false like used for the codeql build, it still builds the test assemblies because they are referenced from the sln.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't think it uses the sln, but instead uses:

<ItemGroup Condition="'$(DotNetBuildFromSource)' != 'true' and '$(DotNetBuild)' != 'true'">
<ProjectToBuild Include="$(RepoRoot)src\**\*.csproj" Exclude="$(RepoRoot)src\Aspire.ProjectTemplates\templates\**\*.csproj" />
<ProjectToBuild Include="$(RepoRoot)eng\dcppack\**\*.csproj" />
<ProjectToBuild Include="$(RepoRoot)eng\dashboardpack\**\*.csproj" />
<ProjectToBuild Include="$(RepoRoot)playground\**\*.csproj" />
<ProjectToBuild Include="$(RepoRoot)tests\**\*.csproj" />
</ItemGroup>

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh then we should definitely make building the tests conditional. I'll open an issue to track that work.

@radical
Copy link
Member Author

radical commented Sep 18, 2024

@joperezr this needs a review.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

This is good enough to unblock the codeql build. But I'd like to understand why we are building tests at all in that build.

@radical
Copy link
Member Author

radical commented Sep 18, 2024

This is good enough to unblock the codeql build. But I'd like to understand why we are building tests at all in that build.

Opened this to track work to make building the tests conditional - #5769

@radical radical merged commit a999113 into dotnet:main Sep 18, 2024
11 checks passed
@radical radical deleted the fix-codeql-build branch September 18, 2024 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] codeql build broken
2 participants