Precommit hook type-checking fails for release branches.

[pixelzoom]

When trying to patch function-builder 1.2, I had big problems with commiting anything to that branch. grunt commit appeared to succeed, but was actually failing pre-commit hooks. And it was failing because it was running tsc for all repos , not just the repos that are identified in function-builder's dependencies.json.

So 2 problems:

(1) git commit command line is failing silently, not showing pre-commit hook failures. Running pre-commit hooks manually with node ../perennial/js/scripts/hook-pre-commit.js --console showed lots of failures.

(2) Running tsc for all repos as part of pre-commit hooks is a timebomb for maintenance releases. It will not work. Repos that are still on master (not part of the release) will fail to find stuff in repos that are on a sha (part of the release).

My workaround to move forward with function-builder 1.2 was to commit from WebStorm with githooks disabled, not a good practice.

[jonathanolson]

Notably for (2), the automated maintenance tools pass --no-verify to commits.

[samreid]

How should this issue be solved? Adding a check in the precommit hooks to detect whether a repo is a common one or not is insufficient--checking all downstream dependencies would still yield incorrect results if some repos are checked out to a maintenance SHA and some are still in master.

[zepumph]

Since our type checking will check all repos (at least as of now and in the past publications), we should just totally opt out. Should that logic go in the script that is outside of chipper? Is there such a thing anymore, or was it all moved to chipper?

A solution would be to have this line changed to a new file in perennial that will NEVER change, and then have the perennial file conduct the logic of "do I run these?" and if so, forwards to chipper. @samreid I bet you can think of a better solution though.

[samreid]

I considered something like this:

```diff Subject: [PATCH] Update docs --- Index: js/scripts/hook-pre-commit.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/scripts/hook-pre-commit.js b/js/scripts/hook-pre-commit.js --- a/js/scripts/hook-pre-commit.js (revision 17d66096430f03ac8bdefe5901baa9588c4e53fa) +++ b/js/scripts/hook-pre-commit.js (date 1669738089353) @@ -36,7 +36,17 @@ const commandLineArguments = process.argv.slice( 2 ); const outputToConsole = commandLineArguments.includes( '--console' ); - const promises = [ 'lint', 'report-media', 'tsc', 'qunit', 'phet-io-api-compare' ].map( task => { + const tasks = [ 'lint', 'report-media', 'tsc', 'qunit', 'phet-io-api-compare' ]; + + // Get the name of the git branch for the current repo + const branch = ( await execute( 'git', [ 'rev-parse', '--abbrev-ref', 'HEAD' ], `../${repo}` ) ).trim(); + + // Only run the tsc step if master branch is checked out + if ( branch === 'master' ) { + tasks.push( 'tsc' ); + } + + const promises = tasks.map( task => { return phetTimingLog.startAsync( task, async () => { const results = await execute( 'node', [ '../chipper/js/scripts/hook-pre-commit-task.js', ```

It only runs tsc if the current repo (the one precommit hooks are running for) is in the master branch. Does this seem it may work?

[pixelzoom]

It only runs tsc if the current repo (the one precommit hooks are running for) is in the master branch. Does this seem it may work?

So you're proposing that we allow potential TypeScript errors on non-master branches? I don't think that's a good idea.

[pixelzoom]

This bit me again today, while patching the 1.2 release branch of geometric-optics-basics. My workaround again was to commit from WebStorm with githooks disabled.

[samreid]

The problem with precommit hooks is that they are repo-specific and monorepo-oriented. We have a polyrepo setup, so trying to fit cross-repo checks into precommit hooks causes problems. Maybe we should abandon git hooks and do something more sophisticated.

UPDATE: Or git hooks can do the modular checks like linting, and we move the type checking to another layer.

[samreid]

@marlitas and I worked on this patch today:

```diff Subject: [PATCH] Remove dog, see https://github.com/phetsims/circuit-construction-kit-common/issues/882 --- Index: js/scripts/hook-pre-commit-task.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/scripts/hook-pre-commit-task.js b/js/scripts/hook-pre-commit-task.js --- a/js/scripts/hook-pre-commit-task.js (revision dd851302fdd85a9eeabc835846b46c8266cd35c7) +++ b/js/scripts/hook-pre-commit-task.js (date 1670349048271) @@ -76,23 +76,45 @@ else if ( command === 'tsc' ) { + // Get the name of the git branch for the current repo + const branch = ( await execute( 'git', [ 'rev-parse', '--abbrev-ref', 'HEAD' ], `../${repo}` ) ).trim(); + + // If the repo being committed is in master, presume that everything is in master. + if ( branch === 'master' ) { - // Run typescript type checker if it exists in the checked-out shas - const results = await execute( 'node', [ '../chipper/js/scripts/absolute-tsc.js', '../chipper/tsconfig/all' ], '../chipper', { - errors: 'resolve' - } ); + // Run typescript type checker if it exists in the checked-out shas + const results = await execute( 'node', [ '../chipper/js/scripts/absolute-tsc.js', '../chipper/tsconfig/all' ], '../chipper', { + errors: 'resolve' + } ); - results.stderr.trim().length > 0 && console.log( results.stderr ); - results.stdout.trim().length > 0 && console.log( results.stdout ); + results.stderr.trim().length > 0 && console.log( results.stderr ); + results.stdout.trim().length > 0 && console.log( results.stdout ); - if ( results.code === 0 ) { - outputToConsole && console.log( 'tsc passed' ); - process.exit( 0 ); - } - else { - outputToConsole && console.log( 'tsc failed' ); - process.exit( 1 ); + if ( results.code === 0 ) { + outputToConsole && console.log( 'tsc passed' ); + process.exit( 0 ); + } + else { + outputToConsole && console.log( 'tsc failed' ); + process.exit( 1 ); + } } + else { + console.log( 'repo was not in master: ' + branch ); + + // Find the dependencies that may need to be tested + const activeRunnables = getRepoList( 'active-runnables' ); + const reposToTest = activeRunnables.filter( activeRunnable => getPhetLibs( activeRunnable ).includes( repo ) ); + + // We know the repo we are committing is not in master, so only type check other dependencies that are + // also not in master + const reposNotInMaster = reposToTest.filter( async repo => { + const branch = ( await execute( 'git', [ 'rev-parse', '--abbrev-ref', 'HEAD' ], `../${repo}` ) ).trim(); + return branch !== 'master'; + } ); + + console.log( reposNotInMaster ); + } } else if ( command === 'qunit' ) { ```

The main idea is that:

• If the repo being committed is in master, then we assume that all repos are checked out to master, and therefore we can run the tsconfig/all file as we have been doing.
• If the repo being committed is not in master, then we trace all usages (downstream dependencies) that use the repo, and only invoke the type checker in the ones that are not in master.

Example 1: let's say we are type checking function-builder in branch 1.2. Then it would detect the downstream dependencies as "function-builder". It would detect that it is not in master. Then it would run type checking on function-builder/tsconfig.json. Example 2: type checking sun#function-builder-1.2. Then it would detect the downstream dependencies as "function-builder" and "sun". It would detect that neither of those are in master. Then it would run type checking on function-builder/tsconfig.json and sun/tsconfig.json. Note that this will duplicate work since both of those tsc commands will type check axon. The worst case scenario is that all dependencies for the sim are runnables, and hence it runs tsc N times, where N is the number of dependencies. Maybe an alternative is to find the "most downstream" dependency and only run the type checking on that? But would that miss things?

Does this seem complete and correct? Also, @marlitas and I realized that this may be difficult to maintenance release into existing typescript sims because the precommit hooks (in chipper) are set up in a very different way--not parallelized or split up into hook-pre-commit-task.js

This all feels overcomplicated. Would it be acceptable to say "if you are not in master, then the precommit hooks will not run the type checking for you, you have to do it yourself"? And we would disable type checking for repos not in master? We are still somewhat protected because grunt will still tell you if you have a type error before you build a dev version, RC or production version.

We are at a good point to check in with @pixelzoom and @zepumph.

[pixelzoom]

The worst case scenario is that all dependencies for the sim are runnables, and hence it runs tsc N times, where N is the number of dependencies.

That sounds like a big potential time increase for maintenance releases. Discuss with @jonathanolson.

Maybe an alternative is to find the "most downstream" dependency and only run the type checking on that? But would that miss things?

I don't know. Try it for many cases?

Also, @marlitas and I realized that this may be difficult to maintenance release into existing typescript sims because the precommit hooks (in chipper) are set up in a very different way--not parallelized or split up into hook-pre-commit-task.js

Not my area. Ask @jonathanolson about maintenance releases.

This all feels overcomplicated.

I agree.

Would it be acceptable to say "if you are not in master, then the precommit hooks will not run the type checking for you, you have to do it yourself"?

Imo, no. Because that won't happen 100% of the time. Someone will skip type checking and commit code that fails tsc. And someone else will discover it later (possibly much later).

And have you actually tried what you're suggesting here? How is running the type-checker manually going to be any different than running the precommit hooks? Isn't it still going to fail in the same way when I have a specific release checked out?

And we would disable type checking for repos not in master? We are still somewhat protected because grunt will still tell you if you have a type error before you build a dev version, RC or production version.

@jonathanolson has the most experience with build and maintenance releases. Recommended to get him involved in this -- so I'm going to assign him to this issue. @jonathanolson your thoughts?

[samreid]

And have you actually tried what you're suggesting here? How is running the type-checker manually going to be any different than running the precommit hooks?

With the "manual" mindset, the precommit hooks would not run automatically, because they are not smart enough to know what SHAs are checked out. Instead, it would be the dev's responsibility to invoke the type checker from the directory of the sim that is checked out, so it will only check that sim and its dependencies instead of trying to check other sims that are still on master (and hence incompatible).

So at the moment, developers are responsible for disabling the precommit hooks themselves.

Happy to wait to hear from @jonathanolson before spending more time on this, or wait until it becomes more of a hassle or priority.

[jonathanolson]

For me, I'm not too worried about running tsc itself directly on release branches. The fact that building the release branch with grunt (a) type-checks all of the relevant repos, and (b) ignores whatever else you have checked out in non-dependent repos... is very sufficient for me.

This is weirdly somewhat safe, because IF someone messes up and makes a commit that breaks types, it will fail a build and not get deployed. This has happened through typescript/npm a few times, and it didn't cause risks of anything. I run lots of build checks for maintenance, so this WILL get caught, and WILL be fine.

I'd vote "don't run tsc on non-master, just grunt to check". If tsc doesn't run on those commits I do manually during maintenance, that's a cherry on top (it's a bit annoying forgetting --no-verify).

[samreid]

One more question before proceeding with this issue. I agree with the remarks above and think we should avoid tsc in precommit hooks when you have a mixed working copy (some stale SHAs checked out and other repos in master). However, @zepumph and I are interested in exploring a workflow where we no longer have a mixed working copy. Instead, we would check out a separate copy for that branch. The pattern is described in https://github.com/phetsims/chipper/issues/1112#issuecomment-1435270612. That idea is not fully explored or vetted, but if we decide we want to go in that direction then it may be preferable and easy to leave tsc enabled in those mini-checkouts since it will only have the appropriate code. Type checks will also run much faster in those mini-checkouts. So maybe we could disable tsc in non-master branches right away, and maybe re-enable it if we go in that "parallel checkouts" direction? @jonathanolson and @zepumph what do you think?

[jonathanolson]

I generally don't like checking code out to modify in those other places. I like working from my main working copy. Parallel checkouts are great, but it's not a great experience to be working across multiple working locations at once.

[zepumph]

Yes, I think that we should write a module/controller to our git hooks with logic that says opt out if not on master. Then when that is in place, it should be just as easy to add to that logic and say "if not and master OR some other set of branches". We can cross that bridge later while still unblocking a very real bummer that is hooks on release branches.

[samreid]

I committed a proposed fix and will request review from someone on my subteam.

[samreid]

Some team members do not yet have a version of git that supports --show-current, so I reverted this for now.

[samreid]

I polled on slack dev-public, and it shows most people now have a git with --show-current

So I think we can un-revert this now. Sorry for the hassle and for not checking this in advance.

[samreid]

I reverted the reverts. Still ready for review for someone on my subteam. And I'll post on slack dev-public so people are aware of this change.

OK I changed the precommit hooks so they only run when master branch is checked out for that repo. Details in https://github.com/phetsims/perennial/issues/276. Sorry for the --show-current hassle, I should have polled the team in advance. That issue is out for review by someone on my subteam.

[samreid]

@jessegreenberg agreed to review, thanks! The decision was that precommit hooks should only run when branch master is checked out, and that was implemented in the 2 commits listed above.

[samreid]

I realized that since these commits are in chipper, any sims we do maintenance on before Feb 27 are not going to have the branch===master guard, since they will check out a stale chipper. Our choices:

• Maintenance release that chipper change to all repos
• Move the branch===master guard somewhere else

[jessegreenberg]

The change to hook-pre-commit.js looks great. I tested it by checking out a feature branch in quadrilateral and making a commit to it and saw that pre-commit hooks were skipped.

Then I tested running pre-commit hooks in a release branch and noticed the same thing @samreid did in https://github.com/phetsims/perennial/issues/276#issuecomment-1449100903.

[samreid]

phet-info holds the precommit hook entry point: https://github.com/phetsims/phet-info/blob/master/git-template-dir/hooks/pre-commit. That repo doesn't seem verisoned at the moment (it is like perennial). So we could add the guard there and instruct devs to reinstall the pre-commit hooks into all their repos?

[jessegreenberg]

Good idea. Yes I think we should do that to avoid a maintenance release for this.

[samreid]

I haven't tested it yet, but perhaps it is something like this:

#!/bin/bash
#-----------------------------------------------------------------------------------------------------------------------
# git pre-commit hooks
#
# Please see https://github.com/phetsims/phet-info/blob/master/doc/phet-development-overview.md#utilities-and-instrumentation-for-development-and-testing
# for installation instructions.
#-----------------------------------------------------------------------------------------------------------------------

# Get the name of the current branch
current_branch=$(git branch --show-current)

# Check if the current branch is "master"
if [[ "$current_branch" == "master" ]]; then
    node ../chipper/js/scripts/hook-pre-commit.js
fi

[samreid]

I tested and found that bash script works well. So I documented it and committed it. @jessegreenberg can you please review? If it seems good, let's instruct the dev team to reinstall precommit hooks following the directions at https://github.com/phetsims/phet-info/blob/master/doc/phet-development-overview.md#utilities-and-instrumentation-for-development-and-testing

Should we keep the additional guard in https://github.com/phetsims/chipper/commit/94338c7fd7a793f2d00397681bd19cd9b85b219b ?

[jessegreenberg]

Thanks! I tested the change and verified that works well.

Should we keep the additional guard

I removed them again in the above commits. I think it is most clear to not have this work done twice. Also, it is nice that now you can still run hook-pre-commit.js on non-master branches if you want to, but release and feature branch commits will not be blocked.

[samreid]

I messaged the dev team:

The precommit hooks have been revised so they no longer automatically run on non-master working copies. Please pull-all, then follow the instructions here to reinstall your git hooks: https://github.com/phetsims/phet-info/blob/master/doc/phet-development-overview.md#utilities-and-instrumentation-for-development-and-testing. Details at https://github.com/phetsims/perennial/issues/276

Closing.