Skip to content

Commit

Permalink
phan: Allow deleting baseline files (#36857)
Browse files Browse the repository at this point in the history
In many cases it's pointless to keep around the empty baseline files,
ideally they should never be repopulated. Update the tooling to allow
for these to be deleted, in which case they won't be re-created by
`--update-baseline`.

But in case for some reason one does need to be re-created, add a
`--force-update-baseline` option to allow for that.

Also update the GitHub Action checking for baseline changes to handle
these, and also to handle changes in pseudo-project baselines that
wasn't done in #36467.

For the moment I'm only actually removing the empty baselines for stuff
Garage maintains. We can look at the rest in a followup.
  • Loading branch information
anomiex authored Apr 12, 2024
1 parent a9966bf commit 115b3d0
Show file tree
Hide file tree
Showing 18 changed files with 90 additions and 174 deletions.
17 changes: 0 additions & 17 deletions .github/actions/tool-setup/composer-plugin/.phan/baseline.php

This file was deleted.

4 changes: 0 additions & 4 deletions .github/files/lint-project-structure.sh
Original file line number Diff line number Diff line change
Expand Up @@ -221,10 +221,6 @@ for PROJECT in projects/*/*; do
EXIT=1
echo "::error file=$PROJECT/.phan/config.php::Project $SLUG has PHP files but does not contain .phan/config.php. Refer to Static Analysis in docs/monorepo.md."
fi
if [[ ! -e "$PROJECT/.phan/baseline.php" ]]; then
EXIT=1
echo "::error file=$PROJECT/.phan/baseline.php::Project $SLUG has PHP files but does not contain .phan/baseline.php. Refer to Static Analysis in docs/monorepo.md."
fi
fi

# - composer.json must exist.
Expand Down
35 changes: 26 additions & 9 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -328,19 +328,36 @@ jobs:
- name: Check baselines
run: |
# Anything changed (with a side of printing the diff)
if git diff --exit-code --ignore-matching-lines='^ // ' .phan/baseline.php 'projects/*/*/.phan/baseline.php'; then
if git diff --exit-code --ignore-matching-lines='^ // ' -- .phan/baseline.php '*/.phan/baseline.php'; then
exit 0
fi
# Collect which projects changed to suggest the right command.
PROJECTS=()
if ! git diff --exit-code --name-only .phan/baseline.php &>/dev/null; then
PROJECTS+=( 'monorepo' )
fi
for f in $( git -c core.quotepath=off diff --name-only 'projects/*/*/.phan/baseline.php' ); do
SLUG=${f%/.phan/baseline.php}
SLUG=${SLUG#projects/}
PROJECTS+=( "$SLUG" )
for f in $( git -c core.quotepath=off diff --name-only -- .phan/baseline.php '*/.phan/baseline.php' ); do
if [[ "$f" == ".phan/baseline.php" ]]; then
SLUG=monorepo
elif [[ "$f" == projects/*/*/.phan/baseline.php ]]; then
SLUG=${f%/.phan/baseline.php}
SLUG=${SLUG#projects/}
elif SLUG=$( grep -v '^[ \t]*\/\/' .phan/monorepo-pseudo-projects.jsonc | jq -re --arg f "${f%.phan/baseline.php}" 'to_entries[] | select( .value == $f ) | .key' ); then
: # Ok
else
SLUG=
fi
if grep -q 'This baseline has no suppressions' "$f"; then
if [[ -n "$SLUG" ]]; then
echo "::error file=$f::This Phan baseline is now empty (good job!). You may remove it, or if you want to keep it (e.g. if you expect new unfixed issues to be added in the future) you can run \`jetpack phan --update-baseline $SLUG\` to update it."
else
echo "::error file=$f::This Phan baseline is now empty (good job!). You may remove it."
fi
elif [[ -n "$SLUG" ]]; then
PROJECTS+=( "$SLUG" )
else
echo "::error file=$f::This Phan baseline has changed and should be updated. This Action was unable to determine the command needed to update it; please report this to the Garage team."
fi
done
echo "::error::Phan baselines have changed (good job!). Run \`jetpack phan --update-baseline ${PROJECTS[*]}\` to update them."
if [[ ${#PROJECTS[@]} -gt 0 ]]; then
echo "::error::Phan baselines have changed (good job!). Run \`jetpack phan --update-baseline ${PROJECTS[*]}\` to update them."
fi
exit 1
17 changes: 0 additions & 17 deletions .phan/baseline.php

This file was deleted.

2 changes: 1 addition & 1 deletion docs/monorepo.md
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ We use eslint and phpcs to lint JavaScript and PHP code. Projects should comply

### Static Analysis

We use Phan for PHP static analysis.[^1] Configuration for a project resides in the `.phan/config.php` within the project, which should generally build on top of the `.phan/config.base.php` from the monorepo root. A baseline file also resides at `.phan/baseline.php` to allow for incremental fixing of errors.
We use Phan for PHP static analysis.[^1] Configuration for a project resides in the `.phan/config.php` within the project, which should generally build on top of the `.phan/config.base.php` from the monorepo root. A baseline file may also reside at `.phan/baseline.php` to allow for incremental fixing of errors.

Phan in the monorepo should be run locally via [Jetpack's CLI tool](#first-time) as `jetpack phan`. Note that Phan soft-requires the [PHP ast extension](https://pecl.php.net/package/ast); while on Linux installing this is likely as easy as `sudo apt-get install php8.2-ast`, Mac users have reported having trouble.

Expand Down
17 changes: 0 additions & 17 deletions projects/packages/changelogger/.phan/baseline.php

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: removed
Comment: Remove empty Phan baseline. No change to functionality.


17 changes: 0 additions & 17 deletions projects/packages/codesniffer/.phan/baseline.php

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: removed
Comment: Remove empty Phan baseline. No change to functionality.


17 changes: 0 additions & 17 deletions projects/packages/ignorefile/.phan/baseline.php

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: removed
Comment: Remove empty Phan baseline. No change to functionality.


17 changes: 0 additions & 17 deletions projects/packages/phpcs-filter/.phan/baseline.php

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: removed
Comment: Remove empty Phan baseline. No change to functionality.


17 changes: 0 additions & 17 deletions projects/packages/stub-generator/.phan/baseline.php

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: removed
Comment: Remove empty Phan baseline. No change to functionality.


45 changes: 38 additions & 7 deletions tools/cli/commands/phan.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,11 @@ export async function builder( yargs ) {
type: 'boolean',
description: 'Update the Phan baselines.',
} )
.option( 'force-update-baseline', {
type: 'boolean',
description:
'Update the Phan baselines, even if no baseline currently exists. But please try not to use this, fix the issues instead.',
} )
.option( 'no-use-uncommitted-composer-lock', {
type: 'boolean',
description: "Don't use uncommitted composer.lock files.",
Expand Down Expand Up @@ -106,6 +111,9 @@ export async function builder( yargs ) {
description: 'Write the report to this file instead of to standard output.',
} )
.check( argv => {
if ( argv.forceUpdateBaseline ) {
argv.updateBaseline = true;
}
if (
argv.updateBaseline &&
( argv[ 'allow-polyfill-parser' ] || argv[ 'force-polyfill-parser' ] )
Expand Down Expand Up @@ -276,12 +284,6 @@ export async function handler( argv ) {
} else {
phanArgs.push( '--progress-bar' );
}
if ( argv.baseline !== false ) {
phanArgs.push( '--load-baseline=.phan/baseline.php' );
}
if ( argv.updateBaseline ) {
phanArgs.push( '--save-baseline=.phan/baseline.php' );
}
for ( const arg of [
'automatic-fix',
'allow-polyfill-parser',
Expand Down Expand Up @@ -327,7 +329,17 @@ export async function handler( argv ) {

const projectPhanArgs = checkFilesByProject[ project ]
? [ ...phanArgs, '--include-analysis-file-list', checkFilesByProject[ project ].join( ',' ) ]
: phanArgs;
: [ ...phanArgs ];

// Baseline handling depends on whether the baseline exists.
const hasBaseline =
( await fs.access( path.join( cwd, '.phan/baseline.php' ) ).catch( () => false ) ) !== false;
if ( hasBaseline && argv.baseline !== false ) {
projectPhanArgs.push( '--load-baseline=.phan/baseline.php' );
}
if ( ( hasBaseline && argv.updateBaseline ) || argv.forceUpdateBaseline ) {
projectPhanArgs.push( '--save-baseline=.phan/baseline.php' );
}

let sstdout = process.stdout,
sstderr = process.stderr;
Expand Down Expand Up @@ -416,6 +428,25 @@ export async function handler( argv ) {
} );
}
await proc;

// Don't re-create unneeded baselines with --force-update-baseline.
if ( ! hasBaseline && argv.forceUpdateBaseline ) {
if ( argv.v ) {
sstderr.write(
'Removing that baseline, no issues were reported so it should be empty.\n'
);
}
try {
await fs.unlink( path.join( cwd, '.phan/baseline.php' ) );
} catch ( e ) {
if ( argv.v ) {
sstderr.write(
// prettier-ignore
`Failed to unlink ${ path.join( cwd, '.phan/baseline.php' ) }: ${ e.message }\n`
);
}
}
}
} catch ( e ) {
let json;
try {
Expand Down
17 changes: 0 additions & 17 deletions tools/cli/helpers/doc-parser/.phan/baseline.php

This file was deleted.

17 changes: 0 additions & 17 deletions tools/e2e-commons/.phan/baseline.php

This file was deleted.

0 comments on commit 115b3d0

Please sign in to comment.