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 extension validation step to CI #2756

Closed
wants to merge 15 commits into from
Closed

Add extension validation step to CI #2756

wants to merge 15 commits into from

Conversation

satvu
Copy link
Member

@satvu satvu commented Oct 7, 2024

Issue describing the changes in this PR

resolves #2237

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Documentation issue linked to PR
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • I have added all required tests (Unit tests, E2E tests)

Additional information

This PR introduces a target imported by each extension library that will generate a .csproj file referencing their respective WebJobs extension package. This will allow CI to build and restore these packages, producing a project.assets.json file that can be scanned by Component Governance. Full context in the linked issue.

Important Change for Contributors

WebJobs extensions would no longer be referenced in Properties/AssemblyInfo.cs - they have been moved to the .csproj file of each extension as an MS Build property. To update the WebJobs extension package, please update this new MS Build property, WebJobsExtension.

Testing Locally

  1. Switch to this branch
  2. Go into any extension under /extensions and build the project.
  3. Navigate to bin/Debug/net8.0/ExtensionValidation to see the generated file.

Copy link
Contributor

@jviau jviau left a comment

Choose a reason for hiding this comment

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

Looks great. Just some organizational comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

small nit on naming: Directory.Build.targets (lowercase t)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this to eng/build? Also recommend naming WorkerExtensions.targets or something similar - so we can put all extension related targets into it.

Copy link
Contributor

Choose a reason for hiding this comment

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

For importing, in the root Directory.Build.props, add this:

<PropertyGroup>
  <RepoRoot>$(MSBuildThisFileDirectory)</RepoRoot>
  <EngRoot>$(RepoRoot)eng/</EngRoot>
  <TargetsRoot>$(EngRoot)build/</TargetsRoot>
</PropertyGroup>

You can then use $(TargetsRoot)WorkerExtensions.targets to import this file in the Directory.Build.targets you are adding.


<Import Project="$(MSBuildThisFileDirectory)extensionValidation/GenerateExtensionValidationProjects.targets" />

<Target Name="AddWebJobsExtensionInformation" BeforeTargets="CoreCompile" Condition="'@(WebJobsExtension)' != ''">
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: move this target into GenerateExtensionValidationProjects.targets

</Target>

<Target Name="BuildGeneratedExtensionProject" AfterTargets="GenerateExtensionProject" Condition="'@(WebJobsExtension)' != ''">
<MSBuild Projects="$(IntermediateOutputPath)\ExtensionValidation\ExtensionValidation.csproj" Targets="Restore;Build" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Small nit: use forward slashes /. Also $(IntermediateOutputPath) already includes a trailing slash.

I think we only need to restore to get the project.assets.json, correct?

Suggested change
<MSBuild Projects="$(IntermediateOutputPath)\ExtensionValidation\ExtensionValidation.csproj" Targets="Restore;Build" />
<MSBuild Projects="$(IntermediateOutputPath)ExtensionValidation/ExtensionValidation.csproj" Targets="Restore" />

<Target Name="GenerateExtensionProject" AfterTargets="Compile" Condition="'@(WebJobsExtension)' != ''">
<MakeDir Directories="$(IntermediateOutputPath)ExtensionValidation" />
<WriteLinesToFile
File="$(IntermediateOutputPath)ExtensionValidation\ExtensionValidation.csproj"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
File="$(IntermediateOutputPath)ExtensionValidation\ExtensionValidation.csproj"
File="$(IntermediateOutputPath)ExtensionValidation/ExtensionValidation.csproj"

<Target Name="GenerateExtensionProject" AfterTargets="Compile" Condition="'@(WebJobsExtension)' != ''">
<MakeDir Directories="$(IntermediateOutputPath)ExtensionValidation" />
<WriteLinesToFile
File="$(IntermediateOutputPath)ExtensionValidation\ExtensionValidation.csproj"
Copy link
Contributor

Choose a reason for hiding this comment

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

You could pull $(IntermediateOutputPath)ExtensionValidation/ExtensionValidation.csproj into a property for re-use.

Lines="$([System.IO.File]::ReadAllText($(_ExtensionProjectTemplate))
.Replace('$PackageName$', '%(WebJobsExtension.Identity)')
.Replace('$PackageVersion$', '%(WebJobsExtension.Version)'))"
Overwrite="true" />
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine for now, but we may want to do some work for incremental build support. There are a few ways to do it, but primarily you want to only write to files if their contents have actually changed. By not writing to the csproj if it has not changed, restore will no-op as it will be already up to date.

@satvu
Copy link
Member Author

satvu commented Oct 9, 2024

Closing this as we are going to split this PR into two

@satvu satvu closed this Oct 9, 2024
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.

Add extension dependency validation step to CI
2 participants