Reuse apps already published in the BC artifact database - #9816
Draft
Groenbech96 wants to merge 6 commits into
Draft
Reuse apps already published in the BC artifact database#9816Groenbech96 wants to merge 6 commits into
Groenbech96 wants to merge 6 commits into
Conversation
Test builds unpublish every app from the restored artifact database and republish ~300 of them before a single test runs. The artifact is built from a known BCApps commit, which the NAV build now records in the artifact manifest as "bcAppsCommit". Use it: diff that commit against the commit being built, map the changed files to apps via the existing dependency graph, and only unpublish/republish those apps and their dependents. Off by default - enable with "useArtifactBaseline": true in .github/AL-Go-Settings.json. Anything that cannot be proven safe falls back to today's behavior: no commit in the manifest, an unreachable commit, a build mode other than Default, a fullBuildPatterns hit, an unmappable src file, or a change set covering most of the apps. See build/scripts/ArtifactBaseline.md. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
Build: scripts & configs
Contributor
|
Could not find a linked ADO work item. Please link one by using the pattern 'AB#' followed by the relevant work item number. You may use the 'Fixes' keyword to automatically resolve the work item when the pull request is merged. E.g. 'Fixes AB#1234' |
…ontainer shape Review found that the feature could never activate, and that fixing that would expose three real correctness problems. Both are addressed here. The guard required BuildMode -eq 'Default', but no container-building project uses that mode: the test projects build in IntegrationTests / UncategorizedTests / LegacyTestsBucket1 / LegacyTestsBucket2. Reuse was therefore skipped in 100% of CI builds. The guard now derives the unsafe modes from the settings - any build mode whose conditionalSettings define preprocessorSymbols, which today means 'Clean' - and a regression test asserts the four real modes are allowed. Retained apps are now uninstalled but left published and synchronized, which is the state AL-Go's publish step produces today. This keeps two invariants the previous version broke once reuse could actually happen: - ImportTestDataInBcContainer.ps1 keeps control of install order. The Legacy test type installs base apps + DemoTool, generates demo data, and only then installs the rest; leaving apps installed made Install-BaseAppsForDemoTool a silent no-op. - Test discovery is unchanged. Every test project sets runTestsInAllInstalledTestApps, so the executed test set is derived from the INSTALLED apps. Further correctness fixes: - Identify apps by publisher + name. Several names are not unique in this repository (Tests-Local in the BE/MX/W1 layers, Data Archive in src/Apps and src/System Application), and name-only matching could skip publishing an app the container never held. - Always unpublish the apps that must never be in a test container. The list moved into the module so the container hook and the publish hook agree on it; both entries are BCApps apps, so they previously looked like ordinary reusable apps - and Prevent Metadata Updates Library changes runtime behavior. - Treat .github/** and project settings.json as full-build triggers. They change how apps compile but match no fullBuildPattern and map to no app. - Stamp the state with the container name and run/attempt, and ignore state that does not match. The publish hook runs in a separate process against a shared temp folder. - Ignore BCAPPS_ARTIFACT_BASELINE_COMMIT in CI so the manifest is the only source of truth there. - Only fetch a missing commit into an already-shallow clone, so a developer's complete clone is never made shallow. - Look up bcContainerHelperConfig without constraining the scope, and wrap the whole reuse block in try/catch so it can never introduce a failure the old code did not have. Tests: 53 (was 35), including the build-mode regression, publisher collisions, the exclusion list, stale-state rejection and an end-to-end resolve. The hook smoke test now runs with LegacyTestsBucket1 instead of Default - the fixture that hid the bug. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The reuse decision was built on BuildOptimization's Get-AffectedApps and fullBuildPatterns. Both answer a different question - "which tests must we run?" - where a change in a dependency can change a dependent's behaviour, so expanding downstream is correct there. "Which published binaries are still valid?" is not that question. An app whose own source did not change has identical source at both commits, is already published, installed and synchronized in the restored artifact database, and still resolves its dependencies because BCApps pins them at <major>.<minor>.0.0. Measured on a real 16-commit, 3-day window (1e77e9c..28ff664, 432 changed files, 841 apps): expanding to dependents marks 841 of 841 apps, because src/Layers/W1/BaseApp changed and everything depends on Base Application. Attributing each file to the app that owns it marks 10. Base Application changes in nearly every multi-day window, so the previous logic would have fired approximately never. Changes: - Get-ChangedAppNames attributes each changed file to exactly one app and no longer calls Get-AffectedApps or Test-FullBuildPatternsMatch. - Country layers are resolved through the GDL view chain. A layer folder has no app.json of its own; PreCompileApp.ps1 composes src/Views/<CC> from the chain in src/Layers/.config/views_config.json. A change in layer L now marks its app only for a build of a country whose chain contains L, so src/Layers/DK affects the DK container's Base Application but not W1's. Ignoring country layers altogether would publish a stale localized Base Application. - The country is taken from the artifact url, which is the country the container is built for. - Paths that cannot invalidate an already-published binary are listed explicitly: rulesets and disabled-test lists are diagnostic inputs, and the artifact/platform pins fetch a NEWER artifact carrying a newer bcAppsCommit, which makes the diff smaller. Treating those as a full-refresh trigger meant the weekly UpdateBCArtifactVersion commit disabled the optimization. - Unattributable and unrecognized paths still fall back to a full clean. Also corrects the NAV-side reference: the stamp comes from Get-BCAppsCommitSha in Eng\Normal\Lib\SubmodulesHelper.psm1 (NAV PR 251642), verified present in the published sandbox/29.0.53247.0/w1 and /dk artifacts. Tests: 65 pass in ArtifactBaseline.Test.ps1, covering chain resolution, the no-dependent-expansion contract, per-country layer attribution and the inert path list. Full build/scripts suite 181/182 - the remaining failure is the pre-existing BuildOptimization "50 vs 51 affected apps" assertion, which fails identically on an untouched main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 26e96054-4eb6-4b68-8d9a-4aee2f5b27df
'Run PS Tests' only runs on PRs that touch build/**, so nothing exercises this assertion in between. The hard-coded "50 affected apps" silently rotted when Payables Agent was added to the E-Document dependency set, and the test now fails on an untouched main - blocking the next build-scripts PR that comes along, which happens to be this one. Bumping the number to 51 would just move the trip wire. Assert what the test actually means instead: the change reaches E-Document Core, it does not fan out to the whole repository, and every app in the affected set genuinely depends on E-Document Core. That is stronger than a count and cannot drift when an app is added or removed. Verified: the full build/scripts suite is 182/182 with this change, and the invariant holds for all 51 apps currently in the set. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 26e96054-4eb6-4b68-8d9a-4aee2f5b27df
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| $configPath = Join-Path $BaseFolder ($script:LayersConfigPath -replace '/', '\') | ||
| if (-not (Test-Path $configPath)) { | ||
| return @() |
| $config = Get-Content -Path $configPath -Raw | ConvertFrom-Json | ||
| } | ||
| catch { | ||
| return @() |
| $resolved = $names | Where-Object { $_ -eq $CountryCode.ToUpperInvariant() } | ||
| } | ||
| if (-not $resolved) { | ||
| return @() |
| $guard++ | ||
| } | ||
|
|
||
| return @($chain) |
The selection logic has unit tests, but nothing has ever exercised the hooks end to end: no CI run has created a container with this on, so the container shape, install order, test discovery and the actual time saved are all still unproven. Turn it on for one project rather than repo-wide. Get-ALGoSetting reads $env:settings, which AL-Go populates per project, so this stays scoped to Test Apps W1 and every other project keeps today's behaviour. Test Apps W1 builds in IntegrationTests, UncategorizedTests, LegacyTestsBucket1 and LegacyTestsBucket2, so this also exercises the Legacy buckets - the riskiest path, because they depend on Install-BaseAppsForDemoTool seeing an uninstalled container. This is an experiment, not a claim that it works. What to read from the run: ARTIFACT BASELINE: <n> file(s) changed since artifact commit <sha> ARTIFACT BASELINE: country 'w1', layer chain 'W1' ARTIFACT BASELINE: usable=True ... N of 841 apps changed since the artifact ARTIFACT BASELINE: reusing X app(s) from the artifact, refreshing Y ARTIFACT BASELINE: skipping <k> app(s) already published by the artifact An empty sha, an empty chain, usable=False or N near 841 all mean it fell back and nothing was saved. Test count must match a baseline run exactly, since discovery comes from the installed apps. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 26e96054-4eb6-4b68-8d9a-4aee2f5b27df
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Every
Test Apps *build restores the BC artifact database — in which all apps are already published, installed and synchronized — and then throws it away, so AL-Go republishes ~300 apps one at a time before a single test runs.Install-App Packagesalone takes ~46 minutes.The apps in the artifact are built by the NAV build from a known BCApps commit. The companion NAV change records it in the artifact manifest as
bcAppsCommit, so this PR can diff that commit against the commit being built, work out which apps actually changed, and refresh only those.Retained apps are uninstalled but left published and synchronized — exactly the state AL-Go's publish step produces today. That keeps two invariants intact:
ImportTestDataInBcContainer.ps1retains full control of install order (which theLegacytype depends on), and test discovery — driven byrunTestsInAllInstalledTestAppsfrom the installed apps — is unchanged. Only the publish/sync work is skipped.No extra storage, nothing to upload or invalidate: the artifact is downloaded and restored by the build anyway.
Off by default. Nothing changes until
"useArtifactBaseline": trueis set in.github/AL-Go-Settings.json.Design, rejected storage alternatives and the full fallback matrix:
build/scripts/ArtifactBaseline.md.Only the owning app, never its dependents
The first revision of this PR expanded the refresh set down the dependency graph, via
BuildOptimization'sGet-AffectedApps, and also consultedfullBuildPatterns. Both were the wrong tool.Those exist to answer "which tests must we run?" — a change in a dependency can change a dependent's behaviour, so expanding downstream is correct there. "Which published binaries are still valid?" is a different question. An app whose own source did not change has identical source at both commits, is already published, installed and synchronized in the restored database, and still resolves its dependencies because BCApps pins them at
<major>.<minor>.0.0, so a refreshed Base Application at29.0.2147483647.xstill satisfies it.Measured on a real 16-commit, 3-day window (
1e77e9c2..28ff6645, 432 changed files, 841 apps):src/Layers/W1/BaseAppchanged in that window, and everything depends on Base Application. Base Application changes in nearly every multi-day window, so the previous revision would have fired approximately never — on top offullBuildPatternsvetoing the whole thing onbuild/Packages.jsonandsrc/DisabledTests/*before any app mapping happened.Per container, after intersecting with the apps the container actually holds:
Country layers go through the view chain
A country layer folder carries no
app.json.PreCompileApp.ps1callsNew-GDLView, which composessrc/Views/<CC>from a chain declared insrc/Layers/.config/views_config.json, and compiles the localized app from that view:A change to
src/Layers/<L>/<AppFolder>/**now marks<AppFolder>for republish only whenLis inchain(CC), resolving app identity by walking the chain from the most specific layer back to the base. ForTest Apps W1the RU/IT/ES/NA/APAC layer changes (316 of the 432 files in the example) are genuinely irrelevant; forTest Apps DKasrc/Layers/DK/BaseAppchange does invalidate that container's Base Application. Ignoring country layers wholesale would publish a stale localized Base Application — a correctness bug, not a missed optimization.The country comes from the artifact url (
.../sandbox/29.0.53247.0/w1→w1), which is the country the container is built for.What is deliberately not an invalidator
A retained app is never compiled by this build — its binary is fixed inside the artifact — so no repository-side setting can retroactively change it. Rulesets and disabled-test lists are diagnostic and test-selection inputs: they can fail a build, they cannot make a published
.appwrong.The artifact and platform pins matter most here. In the example window they are the only
build/**and.github/**changes:That is
UpdateBCArtifactVersionhousekeeping, roughly weekly. Bumping a pin downloads a newer artifact carrying a newerbcAppsCommit, which makes the diff smaller. Treating those files as a full-refresh trigger meant the commit that improves this optimization was the one that switched it off.Linked work
Companion change: NAV PR
251642, shipped 2026-07-31. Verified in a published artifact:sandbox/29.0.53247.0/w1and.../dkboth carry"bcAppsCommit": "1e77e9c2e8d4c58e45e32be2876550dbb68b773c", which is a real commit on BCAppsmain. The repo-levelbaseartifact is not stamped, but noTest Appsproject uses it —Test Apps W1setscountry: w1and each localization project sets its own.How I validated this
What I tested and the outcome
build/scripts/tests/ArtifactBaseline.Test.ps1, all passing: commit resolution, country extraction from the artifact url, layer-chain resolution (W1,DK,AT,US, unknown), the explicit no dependent expansion contract, per-country layer attribution in both directions, the inert-path list, unattributable and unrecognized paths, the ratio guard, the unpublish/keep split, stale-state rejection and every fallback.build/scriptssuite: 181/182. The one failure is the pre-existingBuildOptimization"50 vs 51 affected apps" assertion — it fails identically on an untouchedmain(a newly addedPayables Agentapp), and is untouched here.w1manifest, diffed againstmain, produces the 10 apps above and the 8/10 per-container split.app.jsonnames, and that BCApps dependencies are pinned at<major>.<minor>.0.0so a reused app at the artifact's version still satisfies apps built at29.0.2147483647.x.-upgrade/install behavior against artifact-installed apps, must be measured on a realTest Appsbuild before this is enabled.Risk & compatibility
useArtifactBaseline, which is not set. With it unset, both hooks behave exactly as before.git difffailure; a build mode that changes compilation (derived fromconditionalSettings.preprocessorSymbols, i.e.Clean); a changedsrc/file that maps to no app; a layer change whose country has no resolvable view chain; an unrecognized path; or >75% of apps changed.Library - No Transactions,Prevent Metadata Updates Library), are always unpublished, so the container holds what it holds today.UpdateBCArtifactVersionalready keeps it current — and, unlike the previous revision, that bump no longer disables the optimization.