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.

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

[ci] Fix codeql build #5739

Merged
merged 6 commits into from
Sep 18, 2024
Merged

[ci] Fix codeql build #5739

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9409337
[ci] Fix codeql build
radical Sep 16, 2024
6afc263
[tests] Correctly skip installing playwright dependencies for codeql …
radical Sep 16, 2024
4f1c727
Merge remote-tracking branch 'origin/main' into fix-codeql-build
radical Sep 17, 2024
10f3a4d
fix condition
radical Sep 17, 2024
32780e5
debug
radical Sep 17, 2024
6e516cc
Revert the playwright dependencies change
radical Sep 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tests/Directory.Build.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:

aspire/eng/Build.props

Lines 2 to 8 in a8b0bed

<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.

BeforeTargets="ZipTestArchive">

<Error Condition="'$(ExtractTestClassNamesPrefix)' == ''"
Expand Down
2 changes: 1 addition & 1 deletion tests/Shared/Playwright/Playwright.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
<PackageReference Include="Microsoft.Playwright" />
</ItemGroup>

<Target Name="ProvisionBrowsersForPlaywright" AfterTargets="Build" Condition="'$(InstallBrowsersForPlaywright)' == 'true' and @(BrowsersForPlaywrightToInstall->Count()) > 0">
<Target Name="ProvisionBrowsersForPlaywright" AfterTargets="Build" Condition="'$(InstallBrowsersForPlaywright)' == 'true' and @(BrowsersForPlaywrightToInstall->Count()) > 0 and ('$(ArchiveTests)' == 'true' or '$(Test)' != 'false')">
<PropertyGroup>
<_MicrosoftPlaywrightDllPath>$(OutputPath)/Microsoft.Playwright.dll</_MicrosoftPlaywrightDllPath>
</PropertyGroup>
Expand Down
Loading