diff --git a/README.md b/README.md index 3b86f14..a322188 100644 --- a/README.md +++ b/README.md @@ -45,6 +45,7 @@ Inspects file modification trees targeting OpenWrt build recipes: * **PKG_RELEASE Validation:** Enforces correct release values on package changes: new packages must initialize `PKG_RELEASE` to `1`, version updates must reset `PKG_RELEASE` to `1`, and modifications to package files must be accompanied by a version/release change (customizable level: warning/error/disabled). * **UCI Config Validation:** Ensures that any configuration files destined to be installed into `/etc/config/` conform to the standard OpenWrt UCI format (consisting of only `package`, `config`, `option`, `list` statements, comments, and empty lines). * **PKG_NAME Reuse Prevention:** Ensures `PKG_NAME` is not reused inside `call`, `define`, and `eval` Makefile lines, requiring the literal package name instead to keep recipes readable and searchable (default true). +* **Feed Buildbot-Default Guard (`check_buildbot_default`):** Flags newly added `DEFAULT:=...` lines that condition inclusion on `BUILDBOT` inside feed repositories, since this forces a package into buildbot default images without going through the main `openwrt/openwrt` repo's own default-package process (customizable level: warning/error/disabled; skipped entirely for the main repo). ### 3. Patches Check Scans the contribution tree for nested downstream patch targets: diff --git a/cloudflare-worker/src/config.js b/cloudflare-worker/src/config.js index d07da22..5c24c12 100644 --- a/cloudflare-worker/src/config.js +++ b/cloudflare-worker/src/config.js @@ -34,6 +34,7 @@ export const DEFAULT_CONFIG = { check_missing_colon: true, check_makefile_indentation: true, check_pkg_name_reuse: true, + check_buildbot_default: 'warning', // Patches Check features check_patch_headers: true, diff --git a/cloudflare-worker/src/index.js b/cloudflare-worker/src/index.js index 3895c5e..69997e1 100644 --- a/cloudflare-worker/src/index.js +++ b/cloudflare-worker/src/index.js @@ -606,10 +606,10 @@ async function handleWebhook(request, env) { } else { const fetchFileContent = (path) => fetchFileContentCached(path, sha); - const reportMakefile = validateMakefileContext(fullCommit, commitPatch, CONFIG, state); + const reportMakefile = validateMakefileContext(fullCommit, commitPatch, CONFIG, state, repoFullname); if (isBackportPr && item.upstreamPatch) { const upstreamCommit = { commit: { message: getCommitMessageFromPatch(item.upstreamPatch) } }; - const reportUpstreamMakefile = validateMakefileContext(upstreamCommit, item.upstreamPatch, CONFIG, { isNewPackage: false, isDroppedPackage: false }); + const reportUpstreamMakefile = validateMakefileContext(upstreamCommit, item.upstreamPatch, CONFIG, { isNewPackage: false, isDroppedPackage: false }, repoFullname); reportMakefile.errors = reportMakefile.errors.filter(err => !reportUpstreamMakefile.errors.includes(err)); reportMakefile.warnings = reportMakefile.warnings.filter(warn => !reportUpstreamMakefile.warnings.includes(warn)); reportMakefile.successes.push("✅ Filtered out style/packaging issues already present in upstream commit"); @@ -682,7 +682,7 @@ async function handleWebhook(request, env) { // 2. Makefiles (PR-Wide) const fetchFileContent = (path) => fetchFileContentCached(path, data.pull_request.head.sha); - const reportMakefile = validateMakefileContext(virtualCommit, prPatch, CONFIG, state); + const reportMakefile = validateMakefileContext(virtualCommit, prPatch, CONFIG, state, repoFullname); const reportUci = await validateUciConfigs(prPatch, CONFIG, fetchFileContent); makefileOutputText += `#### Pull Request Overall Diff:\n`; diff --git a/cloudflare-worker/src/validators.js b/cloudflare-worker/src/validators.js index 79485fe..8d9e18c 100644 --- a/cloudflare-worker/src/validators.js +++ b/cloudflare-worker/src/validators.js @@ -447,7 +447,12 @@ export function matchVersionString(subject, version) { } } -export function validateMakefileContext(fullCommit, commitPatch, CONFIG, state) { +// The only repository where a package's own DEFAULT:=y line is allowed to +// pull it into buildbot default images; everywhere else (feeds) that +// decision belongs to the main repo, not the package itself. +const MAIN_REPO_FULLNAME = 'openwrt/openwrt'; + +export function validateMakefileContext(fullCommit, commitPatch, CONFIG, state, repoFullname) { const errors = []; const successes = []; const warnings = []; @@ -869,6 +874,81 @@ export function validateMakefileContext(fullCommit, commitPatch, CONFIG, state) } } + if ( + CONFIG.check_buildbot_default && + CONFIG.check_buildbot_default !== 'disabled' && + repoFullname !== MAIN_REPO_FULLNAME + ) { + const fileDiffs = commitPatch.split(/^diff --git /m); + const buildbotDefaultMessages = []; + + for (const fileDiff of fileDiffs) { + const fileMatch = fileDiff.match(/^\+\+\+\s+b\/(.*)$/m); + if (!fileMatch) continue; + const filePath = fileMatch[1].trim(); + const isMakefile = filePath.endsWith('/Makefile') || filePath === 'Makefile'; + if (!isMakefile) continue; + + // Track which Package/* block an added line lives in, purely to + // produce a friendlier message. Re-derived from hunk header context + // (like the conffiles check) so state doesn't leak across hunks. + let currentPackage = ''; + const lines = fileDiff.split('\n'); + + for (const line of lines) { + if (/^@@/.test(line)) { + const hunkContextMatch = line.match(/^@@[^@]*@@\s*(.*)$/); + const hunkContext = hunkContextMatch ? hunkContextMatch[1] : ''; + if (/\bendef\b/.test(hunkContext)) { + currentPackage = ''; + } else { + const hunkDefineMatch = hunkContext.match(/^define\s+(Package\/\S+)/); + currentPackage = hunkDefineMatch ? hunkDefineMatch[1] : ''; + } + continue; + } + + if (line.startsWith('+') || line.startsWith(' ')) { + const contentLine = line.slice(1); + const defineMatch = contentLine.match(/^define\s+(Package\/\S+)/); + if (defineMatch) { + currentPackage = defineMatch[1]; + continue; + } + if (contentLine.match(/^endef/)) { + currentPackage = ''; + continue; + } + + if (line.startsWith('+')) { + const trimmed = contentLine.trim(); + if (trimmed.startsWith('#')) continue; + + if (/^DEFAULT\s*[:+]?=.*\bBUILDBOT\b/.test(trimmed)) { + const pkgLabel = currentPackage ? ` inside '${currentPackage}'` : ''; + buildbotDefaultMessages.push( + `- Makefile line '${trimmed}'${pkgLabel} conditions DEFAULT on BUILDBOT, forcing this feed package into buildbot default images. Default package selection should be established in the main openwrt/openwrt repository, not sneaked in via a feed package's own Makefile.` + ); + } + } + } + } + } + + if (buildbotDefaultMessages.length > 0) { + const isWarning = CONFIG.check_buildbot_default === 'warning'; + buildbotDefaultMessages.forEach(msg => { + if (isWarning) { + warnings.push(msg); + } else { + errors.push(msg); + } + }); + } else { + successes.push("✅ No feed package forces its own inclusion into buildbot default images via DEFAULT+BUILDBOT"); + } + } + if (CONFIG.check_pkg_name_reuse) { const fileDiffs = commitPatch.split(/^diff --git /m); let pkgNameCheckRun = false; diff --git a/cloudflare-worker/test/validators.test.js b/cloudflare-worker/test/validators.test.js index d2c04a7..1903084 100644 --- a/cloudflare-worker/test/validators.test.js +++ b/cloudflare-worker/test/validators.test.js @@ -28,6 +28,7 @@ const CONFIG = { check_openwrt_meta: true, check_conffiles: true, check_pkg_name_reuse: true, + check_buildbot_default: 'warning', check_patch_headers: true, require_linked_github_account: false, check_openwrt_spelling: true @@ -1612,6 +1613,176 @@ diff --git a/package/utils/foo/Makefile b/package/utils/foo/Makefile assert.strictEqual(res.errors.length, 0); }); + test('warns on DEFAULT conditioned on BUILDBOT in a feed package (issue #4)', () => { + const commit = { commit: { message: 'openssh: add sftp-server DEFAULT' } }; + const patch = ` +diff --git a/net/openssh/Makefile b/net/openssh/Makefile +--- a/net/openssh/Makefile ++++ b/net/openssh/Makefile ++define Package/openssh-sftp-server ++ $(call Package/openssh/Default) ++ TITLE+= SFTP server ++ DEFAULT:=y if (BUILDBOT && !SMALL_FLASH) ++endef + `; + const state = { isNewPackage: false, isDroppedPackage: false }; + const testConfig = { + ...CONFIG, + check_openwrt_meta: false, + check_conffiles: false, + check_crlf: false, + check_pkg_version: false, + check_trailing_newline: false, + check_makefile_indentation: false, + check_pkg_name_reuse: false, + check_buildbot_default: 'warning' + }; + const res = validateMakefileContext(commit, patch, testConfig, state, 'openwrt/packages'); + assert.strictEqual(res.errors.length, 0); + assert.strictEqual(res.warnings.length, 1); + assert.ok(res.warnings[0].includes("DEFAULT:=y if (BUILDBOT && !SMALL_FLASH)")); + assert.ok(res.warnings[0].includes("inside 'Package/openssh-sftp-server'")); + }); + + test('treats check_buildbot_default: true as a hard error', () => { + const commit = { commit: { message: 'foo: add DEFAULT' } }; + const patch = ` +diff --git a/utils/foo/Makefile b/utils/foo/Makefile +--- a/utils/foo/Makefile ++++ b/utils/foo/Makefile ++define Package/foo ++ DEFAULT:=y if BUILDBOT ++endef + `; + const state = { isNewPackage: false, isDroppedPackage: false }; + const testConfig = { + ...CONFIG, + check_openwrt_meta: false, + check_conffiles: false, + check_crlf: false, + check_pkg_version: false, + check_trailing_newline: false, + check_makefile_indentation: false, + check_pkg_name_reuse: false, + check_buildbot_default: true + }; + const res = validateMakefileContext(commit, patch, testConfig, state, 'openwrt/packages'); + assert.strictEqual(res.warnings.length, 0); + assert.strictEqual(res.errors.length, 1); + assert.ok(res.errors[0].includes("DEFAULT:=y if BUILDBOT")); + }); + + test('does not flag DEFAULT without a BUILDBOT condition', () => { + const commit = { commit: { message: 'foo: add DEFAULT' } }; + const patch = ` +diff --git a/utils/foo/Makefile b/utils/foo/Makefile +--- a/utils/foo/Makefile ++++ b/utils/foo/Makefile ++define Package/foo ++ DEFAULT:=y if TARGET_x86 ++endef + `; + const state = { isNewPackage: false, isDroppedPackage: false }; + const testConfig = { + ...CONFIG, + check_openwrt_meta: false, + check_conffiles: false, + check_crlf: false, + check_pkg_version: false, + check_trailing_newline: false, + check_makefile_indentation: false, + check_pkg_name_reuse: false, + check_buildbot_default: 'warning' + }; + const res = validateMakefileContext(commit, patch, testConfig, state, 'openwrt/packages'); + assert.strictEqual(res.errors.length, 0); + assert.strictEqual(res.warnings.length, 0); + assert.ok(res.successes.some(s => s.includes("No feed package forces its own inclusion"))); + }); + + test('does not flag DEFAULT+BUILDBOT in the main openwrt/openwrt repo', () => { + const commit = { commit: { message: 'openssh: add sftp-server DEFAULT' } }; + const patch = ` +diff --git a/net/openssh/Makefile b/net/openssh/Makefile +--- a/net/openssh/Makefile ++++ b/net/openssh/Makefile ++define Package/openssh-sftp-server ++ DEFAULT:=y if (BUILDBOT && !SMALL_FLASH) ++endef + `; + const state = { isNewPackage: false, isDroppedPackage: false }; + const testConfig = { + ...CONFIG, + check_openwrt_meta: false, + check_conffiles: false, + check_crlf: false, + check_pkg_version: false, + check_trailing_newline: false, + check_makefile_indentation: false, + check_pkg_name_reuse: false, + check_buildbot_default: 'warning' + }; + const res = validateMakefileContext(commit, patch, testConfig, state, 'openwrt/openwrt'); + assert.strictEqual(res.errors.length, 0); + assert.strictEqual(res.warnings.length, 0); + }); + + test('does not flag pre-existing DEFAULT+BUILDBOT lines left untouched by the diff', () => { + const commit = { commit: { message: 'foo: unrelated tweak' } }; + const patch = ` +diff --git a/utils/owut/Makefile b/utils/owut/Makefile +--- a/utils/owut/Makefile ++++ b/utils/owut/Makefile +@@ -10,7 +10,7 @@ define Package/owut + DEFAULT:=y if (BUILDBOT && !SMALL_FLASH) +- TITLE:=owut - an OpenWrt Upgrade Tool ++ TITLE:=owut - an OpenWrt upgrade tool + endef + `; + const state = { isNewPackage: false, isDroppedPackage: false }; + const testConfig = { + ...CONFIG, + check_openwrt_meta: false, + check_conffiles: false, + check_crlf: false, + check_pkg_version: false, + check_trailing_newline: false, + check_makefile_indentation: false, + check_pkg_name_reuse: false, + check_buildbot_default: 'warning' + }; + const res = validateMakefileContext(commit, patch, testConfig, state, 'openwrt/packages'); + assert.strictEqual(res.errors.length, 0); + assert.strictEqual(res.warnings.length, 0); + }); + + test('respects check_buildbot_default: false configuration option', () => { + const commit = { commit: { message: 'foo: add DEFAULT' } }; + const patch = ` +diff --git a/utils/foo/Makefile b/utils/foo/Makefile +--- a/utils/foo/Makefile ++++ b/utils/foo/Makefile ++define Package/foo ++ DEFAULT:=y if BUILDBOT ++endef + `; + const state = { isNewPackage: false, isDroppedPackage: false }; + const testConfig = { + ...CONFIG, + check_openwrt_meta: false, + check_conffiles: false, + check_crlf: false, + check_pkg_version: false, + check_trailing_newline: false, + check_makefile_indentation: false, + check_pkg_name_reuse: false, + check_buildbot_default: false + }; + const res = validateMakefileContext(commit, patch, testConfig, state, 'openwrt/packages'); + assert.strictEqual(res.errors.length, 0); + assert.strictEqual(res.warnings.length, 0); + }); + test('rejects reuse of PKG_NAME in call, define, and eval lines', () => { const commit = { commit: { message: 'foo: test' } }; const patch = `