build: Remove separate bundle size pipeline #24344

alexvy86 · 2025-04-11T22:37:04Z

Description

Removes the separate pipeline we use in the public project to calculate bundle sizes (and test coverage, which was removed from it by mistake recently), and starts leveraging the main build pipeline of the client release group for that (still in the public project).

This requires that we start running the main build pipeline in the public project for every commit in main, which we don't do today but it's easily configurable (it would replace the corresponding run of the bundle size pipeline; would take longer, but would not be a blocker on the PR gate).

Reviewer Guidance

The review process is outlined on this wiki page.

Note that this does not remove the bundle size pipeline in ADO, just its definition file, so it wouldn't trigger anymore. But as long as we don't delete it in ADO, reverting this PR (and disabling build-client from running for every commit in main in the public project, if we already did that) would get us back to the current state.

alexvy86 · 2025-04-11T23:42:01Z

Bundle analysis and code coverage reports correctly published as artifacts for runs of the Build - client packages: https://dev.azure.com/fluidframework/public/_build/results?buildId=332354&view=artifacts&pathAsName=false&type=publishedArtifacts.

alexvy86 · 2025-04-18T16:46:10Z

tools/pipelines/build-build-tools.yml

@@ -117,6 +117,5 @@ extends:
    taskLintName: ci:eslint
    poolBuild: Large-eastus2
    checkoutSubmodules: true
-    taskBundleAnalysis: false


The parameter went away because now the "generic" build-npm-package.yml template doesn't do bundle analysis, only build-npm-client-package.yml for the build pipeline of the client release group does. No other build was leveraging this possibility of the "generic" template, and the bundle sizes for everything else we build isn't as critical, so I don't think we're losing anything important by removing the capability and simplifying that template.

alexvy86 · 2025-04-18T16:47:29Z

tools/pipelines/templates/build-npm-package.yml

-  type: boolean
-  default: false
-
- name: taskPack


The only thing overriding taskPack to false was the bundle size pipeline, so there's no need for it anymore.

alexvy86 · 2025-04-18T16:48:30Z

tools/pipelines/templates/build-npm-package.yml

-                  # At this point we want to publish the artifact with npm-packed packages, and the one with test files,
-                  # but as part of 1ES migration that's now part of templateContext.outputs below.
-
-              # Collect/publish/run bundle analysis


No more bundle analysis in this template so all tasks under this condition just go away.

alexvy86 · 2025-04-18T16:49:42Z

tools/pipelines/templates/build-npm-client-package.yml

-  type: boolean
-  default: false
-
- name: taskPack


The build pipeline for the client release group always runs the packing task. taskPack here was a leftover from when we copied this template from build-npm-client.yml and had no use.

alexvy86 · 2025-04-18T17:20:04Z

tools/pipelines/build-client.yml

+    ${{ if eq(variables['System.TeamProject'], 'internal') }}:
+      # Overriding some variables should only be possible in the internal project
+      releaseBuildOverride: ${{ parameters.releaseBuildOverride }}
+      publishOverride: ${{ parameters.publishOverride }}
+      packageTypesOverride: ${{ parameters.packageTypesOverride }}


Since we don't want to ever release from the public project, now overriding those parameters only has an effect in the internal project. Potential improvement for the future that could make this cleaner: if we had two main pipeline files, one for public and one for internal, each tied to the pipeline in the corresponding project. The logic would still be shared in build-npm-client-package.yml.

alexvy86 · 2025-04-18T18:59:50Z

build-tools/packages/build-cli/src/library/dangerfile.cts

@@ -34,7 +34,7 @@ declare const danger: {
 const adoConstants = {
 	orgUrl: "https://dev.azure.com/fluidframework",
 	projectName: "public",
-	ciBuildDefinitionId: 48,
+	ciBuildDefinitionId: 11, // 11 = Build - client packages ; this definition only exists in the public ADO project


Since the pipelines install build-tools from the repo by default (over the npm version), I think this change becomes effective immediately, we don't need to release build-tools and consume it first.

What's 48? Is it used to build another pipeline and now used to build client package pipeline only?

ciBuildDefinitionId defines which pipeline in ADO is generating the baseline measurements for bundle size and test coverage. The logic in the dangerfile then tries to download artifacts from runs of that pipeline to get the baselines. 48 is the pipeline definitionId for Build - Client bundle size artifacts (see the definitionId=48 in the URL when you click that link), the one we use today to generate the baselines. definitionId=11 corresponds to Build - client packages (in the public project! In internal it's different).

alexvy86 · 2025-04-18T19:14:32Z

tools/pipelines/templates/build-npm-client-package.yml

-                      tagName: '${{ parameters.tagName }}'
-                      interdependencyRange: '${{ parameters.interdependencyRange }}'
-                      packageTypesOverride: '${{ parameters.packageTypesOverride }}'
+              - template: /tools/pipelines/templates/include-install-build-tools.yml@self


Build-tools is installed by the include-set-package-version.yml template included beneath this, but since that one is conditional, we now need to ensure build-tools is installed in all scenarios.

That seems like this should then be an else.
But really it seems like this should be explicit and there should be an option to skip the build tools install in set-package-version.
I notice that the buildToolsVersionToInstall is different for this case and the set-package-version, too.

jason-ha · 2025-04-18T21:39:44Z

tools/pipelines/build-client.yml

-  displayName: Release Build (default = not released)
+  displayName: Release Build (default = not released) (has no effect on the public ADO project)


Feels like we should have two front ends for this otherwise common pipeline.
For public no or limited options.
I would also be very much in favor of having the PR and CI pipeline runs separate. Right now we get that from internal vs. public but we can keep separate using (new definition). One of the things we like to see is that the pipeline never fails for CI. But we expect failure for PR runs.

jason-ha · 2025-04-18T22:08:29Z

tools/pipelines/templates/build-npm-client-package.yml

+              # If we set versions when this runs in the public ADO project,
+              # PRs can end up with a few small differences in the pkgVersion files that look like spurious
+              # changes to bundle size, so skip the update when not in the internal project.
+              - ${{ if eq(variables['System.TeamProject'], 'internal') }}:


double negative to positive
"so only update when in the internal project"

jason-ha · 2025-04-18T22:18:19Z

tools/pipelines/templates/build-npm-client-package.yml

+                  targetPath: $(Build.ArtifactStagingDirectory)/bundleAnalysis
+                  # NOTE: this artifact name is used by build-cli, don't change it without careful consideration
+                  # of what else we need to update
+                  artifactName: bundleAnalysis


relative path to the relevant code that needs to match?

jatgarg · 2025-04-18T22:33:31Z

@alexvy86 But we haven't merged those PRs yet, right? So the baselines that it would generate won't be correct since we keep updating PRs.

The baseline is not a real baseline in this case.

alexvy86 · 2025-04-18T23:19:44Z

But we haven't merged those PRs yet, right? So the baselines that it would generate won't be correct since we keep updating PRs.

@jatgarg I'm not sure I follow. Which PRs are you referring to? If you mean PRs that are in-flight right now, they do end up in an "intermediate world" but the way I'm thinking about those is that after we merge this, their PR builds will try to find the bundle size / test coverage baselines in runs of build - client packages, won't find one because the build - client packages run for the commit from which those PRs forked off of main wasn't generating baselines yet, and I believe in case of "couldn't find a baseline" we just don't try to calculate deltas nor post a comment to the PR? I'm just not sure if the error would be fatal to the build, but I think it wouldn't.

alexvy86 added 7 commits April 11, 2025 15:02

Add webpack as dependency for build in bundle-size-tests

dd15266

Make it build:test

4d863d7

Remove script:false

24e62f3

Use main build pipeline for client for bundle size and test coverage

b33c83a

Remove pipeline and obsolete parameters

7bd886e

Add note

3182ea6

Update build definition in build-cli

53b3940

github-actions bot added base: main area: build area: examples labels Apr 11, 2025

alexvy86 mentioned this pull request Apr 11, 2025

build: Restore tests in pipeline that measures bundle size baselines #24322

Closed

Merge branch 'main' into remove-bundle-size-pipeline

28b998d

github-actions bot removed the area: examples label Apr 18, 2025

alexvy86 commented Apr 18, 2025

View reviewed changes

Comments

f75045d

alexvy86 commented Apr 18, 2025

View reviewed changes

Restore check

d49837f

alexvy86 commented Apr 18, 2025

View reviewed changes

Add build tools installation

6b9e647

alexvy86 commented Apr 18, 2025

View reviewed changes

alexvy86 marked this pull request as ready for review April 18, 2025 20:53

alexvy86 requested review from a team April 18, 2025 20:54

jason-ha reviewed Apr 18, 2025

View reviewed changes

		displayName: Release Build (default = not released)
		displayName: Release Build (default = not released) (has no effect on the public ADO project)

build: Remove separate bundle size pipeline #24344

Are you sure you want to change the base?

build: Remove separate bundle size pipeline #24344

Uh oh!

Conversation

alexvy86 commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Reviewer Guidance

Uh oh!

alexvy86 commented Apr 11, 2025

Uh oh!

alexvy86 Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexvy86 Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexvy86 Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chentong7 Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexvy86 Apr 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jatgarg commented Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexvy86 commented Apr 18, 2025

Uh oh!

Uh oh!

alexvy86 commented Apr 11, 2025 •

edited

Loading

alexvy86 Apr 18, 2025 •

edited

Loading

alexvy86 Apr 18, 2025 •

edited

Loading

alexvy86 Apr 18, 2025 •

edited

Loading

chentong7 Apr 29, 2025 •

edited

Loading

alexvy86 Apr 30, 2025 •

edited

Loading

jatgarg commented Apr 18, 2025 •

edited

Loading