Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to clean the cache during a build #1243

Closed
mattpen opened this issue May 3, 2022 · 16 comments
Closed

How to clean the cache during a build #1243

mattpen opened this issue May 3, 2022 · 16 comments

Comments

@mattpen
Copy link
Contributor

mattpen commented May 3, 2022

Currently, number play is not able to build due to an error in vegas.

Steps to reproduce:

This only occurs on phet-server2 (running RHEL 8). Node version is 16.14.2. This does not occur on macOS 12 with Node 16.14.2.

  1. Checkout number-play 1.0 branch. (vegas b9b7228)
  2. In perennial, run grunt checkout-shas --repo=number-play
  3. In number-play, run grunt.

Result:

[phet-admin@phet-server2 number-play]$ grunt
Running "lint-all" task

Running "report-media" task

Running "clean" task

Running "build" task
Fatal error: tsc failed with code: 1
stdout:
../vegas/js/demo/FiniteChallengesScreenView.ts(69,20): error TS2345: Argument of type 'FiniteStatusBar' is not assignable to parameter of type 'Node'.
  Type 'FiniteStatusBar' is missing the following properties from type 'Node': _id, _instances, _rootedDisplays, _drawables, and 631 more.
../vegas/js/demo/FiniteChallengesScreenView.ts(101,22): error TS2339: Property 'bottom' does not exist on type 'FiniteStatusBar'.
../vegas/js/demo/FiniteChallengesScreenView.ts(127,22): error TS2339: Property 'bottom' does not exist on type 'FiniteStatusBar'.
../vegas/js/demo/InfiniteChallengesScreenView.ts(35,93): error TS2345: Argument of type 'Text' is not assignable to parameter of type 'Node'.
  Type 'Text' is missing the following properties from type 'Node': baseURI, childNodes, firstChild, isConnected, and 44 more.
../vegas/js/demo/InfiniteChallengesScreenView.ts(38,20): error TS2345: Argument of type 'InfiniteStatusBar' is not assignable to parameter of type 'Node'.
  Type 'InfiniteStatusBar' is missing the following properties from type 'Node': _id, _instances, _rootedDisplays, _drawables, and 631 more.
../vegas/js/demo/InfiniteChallengesScreenView.ts(43,22): error TS2339: Property 'bottom' does not exist on type 'InfiniteStatusBar'.
js/game/view/NumberPlayGameLevelNode.ts(81,34): error TS2339: Property 'children' does not exist on type 'InfiniteStatusBar'.
js/game/view/NumberPlayGameLevelNode.ts(85,20): error TS2345: Argument of type 'InfiniteStatusBar' is not assignable to parameter of type 'Node'.
  Type 'InfiniteStatusBar' is missing the following properties from type 'Node': _id, _instances, _rootedDisplays, _drawables, and 631 more.


12 problems found.

stderr:
@mattpen mattpen transferred this issue from phetsims/vegas May 3, 2022
@mattpen
Copy link
Contributor Author

mattpen commented May 3, 2022

A tsc cache clean looks like it fixed the problem.

[phet-admin@phet-server2 number-play]$ node ../chipper/node_modules/typescript/bin/tsc -b
../vegas/js/demo/FiniteChallengesScreenView.ts:69:20 - error TS2345: Argument of type 'FiniteStatusBar' is not assignable to parameter of type 'Node'.
  Type 'FiniteStatusBar' is missing the following properties from type 'Node': _id, _instances, _rootedDisplays, _drawables, and 631 more.

...
Found 8 errors.

[phet-admin@phet-server2 number-play]$ node ../chipper/node_modules/typescript/bin/tsc -b --clean
[phet-admin@phet-server2 number-play]$ node ../chipper/node_modules/typescript/bin/tsc -b
[phet-admin@phet-server2 number-play]$ 

EDIT 8.9.2022

The steps to fix this are:

  1. logon to phet-server2
  2. After ensuring it is not in use, stop the build-server
  3. sudo -i -u phet-admin
  4. cd /data/share/phet/build-server/perennial
  5. Checkout master for all repos, pull all, and delete node_modules in chipper, perennial-alias, and number-play
  6. In perennial grunt checkout-shas --repo=number-play --branch=1.0
  7. cd ../number-play && git checkout 1.0
  8. cd ../chipper && rm -rf dist && cd -
  9. node ../chipper/node_modules/typescript/bin/tsc -b --clean
  10. node ../chipper/node_modules/typescript/bin/tsc -b

If successful, the output should be empty. Then open https://phet.colorado.edu/translate/trigger-build/number-play/{{locale}}/{{translatorId}} in a browser and logon. The locale and translator ID can be found by looking at the latest build-server logs, e.g.

Aug 09 10:32:03 phet-server2.int.colorado.edu build-server[465158]: info: deploy request received, original URL = http://phet.colorado.edu/deploy-html-simulation
Aug 09 10:32:03 phet-server2.int.colorado.edu build-server[465158]: api:"2.0"
Aug 09 10:32:03 phet-server2.int.colorado.edu build-server[465158]: dependencies:"{\"comment\":\"# number-play 1.0.3 Sun Jul 17 2022 13:04:05 GMT-0600 (Mountain Daylight Time)\",\"assert\":{\"sha\":\"767>
Aug 09 10:32:03 phet-server2.int.colorado.edu build-server[465158]: simName:"number-play"
Aug 09 10:32:03 phet-server2.int.colorado.edu build-server[465158]: version:"1.0.3"
Aug 09 10:32:03 phet-server2.int.colorado.edu build-server[465158]: locales:["fa"]
Aug 09 10:32:03 phet-server2.int.colorado.edu build-server[465158]: servers:["production"]
Aug 09 10:32:03 phet-server2.int.colorado.edu build-server[465158]: brands:["phet"]
Aug 09 10:32:03 phet-server2.int.colorado.edu build-server[465158]: translatorId:"641432"

@mattpen
Copy link
Contributor Author

mattpen commented May 3, 2022

@samreid is going to look at options for how we clean the cache during a build.

@samreid
Copy link
Member

samreid commented May 3, 2022

In #1241 we observed that running a monolithic tsc ran much faster. So I feel our options for this are:

  • tsc -b and accept that there will be cache failures like this one (not recommended)
  • tsc -b --clean + tsc -b for each build
  • tsc without project references with incremental: true (might it have caching problems???)
  • tsc without project references with incremental: false.

I would add some timing notes from my mac M1:

CCK AC:
clean tsc -b: 14sec
monolithic tsc with incremental:true: 1.7sec
monolithic tsc with incremental:false: 5.5sec

Here's the monolithic tsconfig I used for this test:

/**
 * Shared defaults for tsconfig files.
 * @author Sam Reid (PhET Interactive Simulations)
 */
{
  "compilerOptions": {

    /* Basic Options */
    "incremental": false,                         /* Enable incremental compilation */
    "target": "ES2020",                          /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017', 'ES2018', 'ES2019', 'ES2020', 'ES2021', or 'ESNEXT'. */
    "module": "ES2020",                          /* Specify module code generation: 'none', 'commonjs', 'amd', 'system', 'umd', 'es2015', 'es2020', or 'ESNext'. */
    // "lib": [],                                   /* Specify library files to be included in the compilation. */
    "allowJs": true,                             /* Allow javascript files to be compiled. */
    "checkJs": false,                            /* Report errors in .js files. */
    // "jsx": "preserve",                           /* Specify JSX code generation: 'preserve', 'react-native', 'react', 'react-jsx' or 'react-jsxdev'. */
    "declaration": false,                         /* Generates corresponding '.d.js' file. */
    // "declarationMap": true,                      /* Generates a sourcemap for each corresponding '.d.js' file. */
    "sourceMap": false,                           /* Generates corresponding '.map' file. */
    "outDir": "./outdir",     /* Redirect output structure to the directory. */
    "rootDir": "../../../",                             /* Specify the root directory of input files. Use to control the output directory structure with --outDir. */
    // "rootDir": "./",                             /* Specify the root directory of input files. Use to control the output directory structure with --outDir. */
    "composite": false,                             /* Enable project compilation */
    // "tsBuildInfoFile": "./",                     /* Specify file to store incremental compilation information */
    // "removeComments": true,                      /* Do not emit comments to output. */
    "noEmit": true,                              /* Do not emit outputs. */
    //    "emitDeclarationOnly": true,
    // "importHelpers": true,                       /* Import emit helpers from 'tslib'. */
    // "downlevelIteration": true,                  /* Provide full support for iterables in 'for-of', spread, and destructuring when targeting 'ES5' or 'ES3'. */
    "isolatedModules": true,                     /* Transpile each file as a separate module (similar to 'ts.transpileModule'). */

    /* Strict Type-Checking Options */
    "strict": true,                              /* Enable all strict type-checking options. */
    // "noImplicitAny": true,                       /* Raise error on expressions and declarations with an implied 'any' type. */
    // "strictNullChecks": true,                    /* Enable strict null checks. */
    // "strictFunctionTypes": true,                 /* Enable strict checking of function types. */
    // "strictBindCallApply": true,                 /* Enable strict 'bind', 'call', and 'apply' methods on functions. */
    // "strictPropertyInitialization": true,        /* Enable strict checking of property initialization in classes. */
    // "noImplicitThis": true,                      /* Raise error on 'this' expressions with an implied 'any' type. */
    // "alwaysStrict": true,                        /* Parse in strict mode and emit "use strict" for each source file. */

    /* Additional Checks */
    // "noUnusedLocals": true,                      /* Report errors on unused locals. */
    // "noUnusedParameters": true,                  /* Report errors on unused parameters. */
    "noImplicitReturns": true,                   /* Report error when not all code paths in function return a value. */
    // "noFallthroughCasesInSwitch": true,          /* Report errors for fallthrough cases in switch statement. */
    // "noUncheckedIndexedAccess": true,            /* Include 'undefined' in index signature results */
    "noImplicitOverride": true,                  /* Ensure overriding members in derived classes are marked with an 'override' modifier. */
    // "noPropertyAccessFromIndexSignature": true,  /* Require undeclared properties from index signatures to use element accesses. */

    /* Module Resolution Options */
    // "moduleResolution": "node",                  /* Specify module resolution strategy: 'node' (Node.js) or 'classic' (TypeScript pre-1.6). */
    // "baseUrl": "./",                             /* Base directory to resolve non-absolute module names. */
    // "paths": {},                                 /* A series of entries which re-map imports to lookup locations relative to the 'baseUrl'. */
    // "rootDirs": [],                              /* List of root folders whose combined content represents the structure of the project at runtime. */
    // "typeRoots": [],                             /* List of folders to include type definitions from. */
    // "types": [],                                 /* Type declaration files to be included in compilation. */
    // "allowSyntheticDefaultImports": true,        /* Allow default imports from modules with no default export. This does not affect code emit, just typechecking. */
    // "esModuleInterop": true,                     /* Enables emit interoperability between CommonJS and ES Modules via creation of namespace objects for all imports. Implies 'allowSyntheticDefaultImports'. */
    // "preserveSymlinks": true,                    /* Do not resolve the real path of symlinks. */
    "allowUmdGlobalAccess": true,                /* Allow accessing UMD globals from modules. */

    /* Source Map Options */
    // "sourceRoot": "",                            /* Specify the location where debugger should locate TypeScript files instead of source locations. */
    // "mapRoot": "",                               /* Specify the location where debugger should locate map files instead of generated locations. */
    // "inlineSourceMap": true,                     /* Emit a single file with source maps instead of having a separate file. */
    // "inlineSources": true,                       /* Emit the source alongside the sourcemaps within a single file; requires '--inlineSourceMap' or '--sourceMap' to be set. */

    /* Experimental Options */
    // "experimentalDecorators": true,              /* Enables experimental support for ES7 decorators. */
    // "emitDecoratorMetadata": true,               /* Enables experimental support for emitting type metadata for decorators. */

    /* Advanced Options */
    "skipLibCheck": true,                           /* Skip type checking of declaration files. */
    "forceConsistentCasingInFileNames": true        /* Disallow inconsistently-cased references to the same file. */
  },
  "files": [
    "../../../circuit-construction-kit-ac/js/circuit-construction-kit-ac-main.ts",
    "../../phet-types.d.ts"
  ]
}

I'd rather not suffer another caching problem, so if we think incremental: true may have the same problems, then let's just move to a monolithic approach. However, this would lose our modularity structures from project references, so maybe should be discussed as a team or subteam first.

@samreid samreid removed their assignment May 3, 2022
@jonathanolson
Copy link
Contributor

  1. We do want to run type-checks during builds
  2. We don't want to rely on Microsoft fixing this quickly (due to responsiveness to other issues)
  3. We do want to report this case to Microsoft (minimal reproducible example?)
  4. We want to keep project references (would not preclude us from including monolithic in addition, project references needed so we don't check out everything)
  5. We don't want to tolerate any non-zero false-negative or false-positive rates (does that mean cleaning every time? disabling incremental:true?)

@pixelzoom
Copy link
Contributor

Since there's nothing wrong with vegas or FiniteStatusBar, I'm going to retitle this issue to reflect what the actual problem is.

@pixelzoom pixelzoom changed the title Build Failure: Argument of type 'FiniteStatusBar' is not assignable to parameter of type 'Node' How to clean the cache during a build May 10, 2022
@samreid
Copy link
Member

samreid commented May 11, 2022

We will probably investigate #1245 first.

@samreid
Copy link
Member

samreid commented May 11, 2022

On hold until #1245 has progress.

@mattpen
Copy link
Contributor Author

mattpen commented May 19, 2022

The original problem popped up on the build server again today. I reran the clean step manually.

@zepumph
Copy link
Member

zepumph commented Jun 28, 2022

This happened again with number play again today, and #1245 is fixed. Bringing back up to dev meeting again.

@samreid samreid removed their assignment Jun 29, 2022
@jessegreenberg
Copy link
Contributor

@zepumph: One fix is to blow away chipper/dist (either manually or with tsc --clean) every build on the build server. This would add a minute or two per build. The way we build and clean may change and so if our process changes we may need to update that here too, creating a maintenance burden. Just deleting chipper/dist would not have this problem, using tsc --clean would.

@jonathanolson is concerned about the increase in time for batch maintenance releases.

@mattpen should we do a maintenance release for number-play since that is the sim that seems to trigger this.
@zepumph: A maintenance release for number-play may not be trivial since things about the build process have changed.

@zepumph: Should we disable type checking for builds on the build-server?

Lets continue this in a sub-group with @samreid @jonathanolson @zepumph.

@mattpen
Copy link
Contributor Author

mattpen commented Aug 9, 2022

This happened again today. Updated manual steps to fix are in #1243 (comment). This takes me about an hour to fix every time it happens, it would be nice if we could maintenance release a fix.

@mattpen
Copy link
Contributor Author

mattpen commented Sep 10, 2022

This happened again last night. I followed the steps in #1243 (comment), but there were additional errors (see below). I'm not quite sure how to fix these, so I wasn't able to get the build working. I restarted the build-server but number play is likely to keep failing.

Can we please prioritize handling this in a sub group, @samreid @jonathanolson @zepumph?

$ node ../chipper/node_modules/typescript/bin/tsc -b
js/common/model/OnesPlayArea.ts:96:19 - error TS2339: Property 'includeInSumProperty' does not exist on type 'PaperNumber'.

96       paperNumber.includeInSumProperty.link( calculateTotalListener );
                     ~~~~~~~~~~~~~~~~~~~~

js/common/model/OnesPlayArea.ts:99:19 - error TS2339: Property 'includeInSumProperty' does not exist on type 'PaperNumber'.

99       paperNumber.includeInSumProperty.unlink( calculateTotalListener );
                     ~~~~~~~~~~~~~~~~~~~~

js/common/model/OnesPlayArea.ts:212:48 - error TS2345: Argument of type '{ targetScale: number; }' is not assignable to parameter of type 'number'.

212     paperNumber.setDestination( origin, false, {
                                                   ~
213       targetScale: scale
    ~~~~~~~~~~~~~~~~~~~~~~~~
214     } );
    ~~~~~

js/common/model/OnesPlayArea.ts:250:77 - error TS2345: Argument of type '{ targetScale: number; }' is not assignable to parameter of type 'number'.

250     paperNumber.setDestination( destinationPosition, options.shouldAnimate, {
                                                                                ~
251       targetScale: NumberPlayConstants.COUNTING_OBJECT_SCALE
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
252     } );
    ~~~~~

js/common/model/OnesPlayArea.ts:276:27 - error TS2339: Property 'includeInSumProperty' does not exist on type 'PaperNumber'.

276       return !paperNumber.includeInSumProperty.value;
                              ~~~~~~~~~~~~~~~~~~~~

js/common/model/OnesPlayArea.ts:299:27 - error TS2339: Property 'includeInSumProperty' does not exist on type 'PaperNumber'.

299       paperNumberToReturn.includeInSumProperty.value = false;
                              ~~~~~~~~~~~~~~~~~~~~

js/common/model/OnesPlayArea.ts:303:57 - error TS2345: Argument of type '{ targetScale: number; }' is not assignable to parameter of type 'number'.

303       paperNumberToReturn.setDestination( origin, true, {
                                                            ~
304         targetScale: scale
    ~~~~~~~~~~~~~~~~~~~~~~~~~~
305       } );
    ~~~~~~~

js/common/model/OnesPlayArea.ts:393:89 - error TS2339: Property 'includeInSumProperty' does not exist on type 'PaperNumber'.

393     let objectsToOrganize = [ ...this.paperNumbers ].filter( paperNumber => paperNumber.includeInSumProperty.value );
                                                                                            ~~~~~~~~~~~~~~~~~~~~

js/common/model/OnesPlayArea.ts:405:79 - error TS2345: Argument of type '{ targetScale: number; }' is not assignable to parameter of type 'number'.

405       objectToOrganize && objectToOrganize.setDestination( destination, true, {
                                                                                  ~
406         targetScale: NumberPlayConstants.COUNTING_OBJECT_SCALE
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
407       } );
    ~~~~~~~

js/common/model/OnesPlayArea.ts:416:58 - error TS2339: Property 'includeInSumProperty' does not exist on type 'PaperNumber'.

416     this.paperNumbers.filter( paperNumber => paperNumber.includeInSumProperty.value ).forEach( paperNumber => {
                                                             ~~~~~~~~~~~~~~~~~~~~

js/common/view/OnesPlayAreaNode.ts:246:43 - error TS2339: Property 'includeInSumProperty' does not exist on type 'PaperNumber'.

246       return !paperNumberNode.paperNumber.includeInSumProperty.value;
                                              ~~~~~~~~~~~~~~~~~~~~

js/common/view/OnesPlayAreaNode.ts:248:66 - error TS2339: Property 'includeInSumProperty' does not exist on type 'PaperNumber'.

248     if ( allPaperNumberNodes.length === 0 || !draggedPaperNumber.includeInSumProperty.value ) {
                                                                     ~~~~~~~~~~~~~~~~~~~~

js/common/view/OnesPlayAreaNode.ts:327:24 - error TS2339: Property 'includeInSumProperty' does not exist on type 'PaperNumber'.

327       if ( paperNumber.includeInSumProperty.value ) {
                           ~~~~~~~~~~~~~~~~~~~~

js/common/view/OnesPlayAreaNode.ts:335:74 - error TS2554: Expected 0 arguments, but got 1.

335         this.onesCreatorPanel.countingCreatorNode.checkTargetVisibility( paperNumberValue );
                                                                             ~~~~~~~~~~~~~~~~

js/common/view/OnesPlayAreaNode.ts:342:72 - error TS2554: Expected 0 arguments, but got 1.

342       this.onesCreatorPanel.countingCreatorNode.checkTargetVisibility( paperNumberValue );
                                                                           ~~~~~~~~~~~~~~~~

js/common/view/OnesPlayAreaNode.ts:358:44 - error TS2339: Property 'includeInSumProperty' does not exist on type 'PaperNumber'.

358       const amountToSubtract = paperNumber.includeInSumProperty.value ? paperNumber.numberValueProperty.value : 0;
                                               ~~~~~~~~~~~~~~~~~~~~

js/common/view/OnesPlayAreaNode.ts:359:19 - error TS2339: Property 'includeInSumProperty' does not exist on type 'PaperNumber'.

359       paperNumber.includeInSumProperty.value = false;
                      ~~~~~~~~~~~~~~~~~~~~

js/common/view/OnesPlayAreaNode.ts:363:58 - error TS2339: Property 'returnAnimationBounds' does not exist on type 'PaperNumber'.

363       targetPosition = targetPosition.minus( paperNumber.returnAnimationBounds.center );
                                                             ~~~~~~~~~~~~~~~~~~~~~

js/common/view/OnesPlayAreaNode.ts:366:57 - error TS2345: Argument of type '{ targetScale: number; targetHandleOpacity: number; }' is not assignable to parameter of type 'number'.

366       paperNumber.setDestination( targetPosition, true, {
                                                            ~
367         targetScale: targetScale,
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
368         targetHandleOpacity: 0
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
369       } );
    ~~~~~~~


Found 19 errors.

@jonathanolson
Copy link
Contributor

Do we have a better option than just clearing chipper/dist? It seems like we might need to do that, and sim builds will take a bit longer.

@mattpen
Copy link
Contributor Author

mattpen commented Sep 26, 2022

This happened again for the ms locale yesterday.

Do we have a better option than just clearing chipper/dist? It seems like we might need to do that, and sim builds will take a bit longer.

I'm not sure, if that's what is needed I can add that to the build server. From what I understand that will make maintenance releases significantly longer. I recall @samreid saying something about not using the tsc cache in master? Maybe arithmetic just needs a maintenance release with an updated chipper sha?

@samreid
Copy link
Member

samreid commented Sep 26, 2022

I reached out on slack to schedule a collaboration meeting.

@samreid
Copy link
Member

samreid commented Sep 27, 2022

@mattpen @chrisklus and I were unable to reproduce this problem on the build server, even trying several sim/sim build combinations. So we made a maintenance release for number play 1.4 which avoids type checking in that chipper sha. Local testing seems OK. Closing until the problem recurs.

@samreid samreid closed this as completed Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants