phetsims / perennial

Maintenance tools that won't change with different versions of chipper checked out
MIT License
2 stars 5 forks source link

Precommit hooks should run from perennial-alias instead of perennial #278

Closed samreid closed 1 year ago

samreid commented 2 years ago

From https://github.com/phetsims/chipper/issues/1286 we identified that the precommit hooks node ../perennial/js/scripts/hook-pre-commit.js are loading files from chipper and aqua:

const lint = require( '../../../chipper/js/grunt/lint' );

// ....
const reportMedia = require( '../../../chipper/js/grunt/reportMedia' );

// ...
if ( fs.existsSync( '../chipper/tsconfig/all/tsconfig.json' ) ) {

// ...
const results = await execute( 'node', [ '../../../chipper/node_modules/typescript/bin/tsc' ], '../chipper/tsconfig/all', {

// ...
const puppeteerQUnit = require( '../../../aqua/js/local/puppeteerQUnit' );

Generally, perennial should not import from chipper. But it is OK for perennial-alias to import from chipper. Rather than move all these processes to perennial, it seems like the precommit hooks should run from perennial-alias. This will also allow us to run precommit hooks while in maintenance SHAs, which we have wanted to do but not had a reliable way to do so.

Heads up to @zepumph and @jonathanolson

zepumph commented 2 years ago

While this is true, and we talked about this on Friday, we also said that it would be buggy if you had sim shas checked out and tried to commit in a repo on master (like perennial or aqua). Does this seem like an important case?

samreid commented 2 years ago

I recall that conversation and wanted to discuss a bit further.

Let's say we have checked out maintenance SHAs for gravity-and-orbits and we want to commit a change to perennial. On the maintenance SHAs chipper/eslint has a rule that requires us to use type instead of interface. But in master, chipper/eslint has a rule that requires us to use interface instead of type. So the precommit hooks will fail. The problem is the inconsistency of having the precommit hooks in perennial run rule sets and implementations from chipper SHAs. The solutions are:

Based on these considerations, I'm planning on doing more things in perennial-alias and only using perennial for the small subset of operations that deal with checking out and managing maintenance SHAs. Specifically, at the moment I have perennial-alias excluded in my IDE and navigate/work in perennial, so I would switch that. Likewise, on the command line, I typically navigate to perennial to run grunt lint-everything but maybe I should do this from perennial.

samreid commented 2 years ago

In today's meeting, we agreed to move precommit hooks, absolute-tsc, and other code that uses ../chipper over to chipper itself.

samreid commented 2 years ago

@marlitas and I moved:

We do not think the build-server commands should be moved. We are uncertain about these remaining usages of ../chipper in perennial:

@jonathanolson what do you recommend for these parts?

marlitas commented 1 year ago

@samreid and I discussed and side issues can be created for ChipperVersion, outputJSAll, Gruntfile/lint if we decide to move them to chipper in the future. Closing.