Build in a temporary directory #4968

cpcallen · 2021-06-30T16:54:45Z

The basics

I branched from develop
My pull request is against develop
My code follows the style guide

The details

Proposed Changes

Modify the build, typings and test npm scripts so that they do not overwrite any checked-in file(s). Specifically, separate build into three separate scripts:
- npm run generate:langfiles, to be invoked manually after making changes to msg/messages.js, will run js_to_json.py to update msg/json/*.json.
- npm run build will do the rest of the build process but write the output to BUILD_DIR (configured to be blockly/built/, but could be anywhere).
- npm run checkin:built will copy the built files from BUILD_DIR to the repository root so they can be committed.
The typings npm script is similarly revised to write to BUILD_DIR, and a checkin:typings script provided to copy the results back to typings/. A checkin script will run checkin:built and checkin:typings.
Modify the scripts run by npm test so that, where they depend on built files, they use the copies in BUILD_DIR instead of the the checked-in ones in the repository root.
Modify the recompile script to run checkin:built and checkin:typings.
Provide clean, clean:build and clean:release scripts to wipe the build/ and dist/ temporary directories, and arrange for these to be executed at appropriate points by the

Behavior Before Change

npm run build and npm test would modify committed files.

Behavior After Change

npm run build and npm test should not modify committed files, but npm checkin can be used to request such modifications.

Reason for Changes

Developer experience. Having npm run build or npm test modify tracked files is surprising and makes it difficult to understand what changes are intentional and which are fallout from previous commits.
Moves us toward being able to remove checked-in build products like blockly_compressed.js and msg/js/*.js from the repository in future if we want to.

Test Coverage

This has been manually tested locally, including by removing all the checked-in build products (that I know about!) via

rm -rf *_compressed.js* msg/js/*.js typings/blockly.d.ts typings/msg/*.d.ts
npm test

but I'm not in a position to fully verify the changes to scripts/gulpfiles/release_tasks.js.

Documentation

TBD. These changes ought to be documented somewhere. Is there documentation for our existing npm scripts somewhere?

Additional Information

Release process

There is a question about how packaging and releasing ought to work. I initially modified the package script to pull from BUILD_DIR instead of the repository root, but after looking at the recompile and publish tasks I changed this back so as to not change how the release process works. I'm not sure which is preferable:

The advantage of the present arrangement, where we package and release checked-in build products, is that it very easy to see (and test) exactly what will be packaged/released in advance of doing the release.
The disadvantage is that we run the risk of packaging and releasing out-of-date build products. This is especially a risk as none of the automated tests actually test the checked-in build products (with one minor exception: prior to this PR, tests/scripts/compile_typings.sh did actually check the generated .d.ts files in typings/ would compile).

Overall I think it would be better to move to a process that does build, test, package, and release all in a single step, but I do not have much experience in this area to refer to and would appreciate input.

Appengine

I've not looked in detail at scripts/gulpfiles/appengine_tasks.js; AFAICT it will still use checked-in build products. I would appreciate input from anyone more familiar with the Appengine deploy procedure about how this should be handled.

Per the gulp documentation[1], globs passed to gulp.src should use '/' as the path separator regardless of the path separator used on whatever OS we are running on. [1] https://gulpjs.com/docs/en/getting-started/explaining-globs#segments-and-separators

Make the destination directories for certain build/package/release steps more easily (and centrally) configurable. This only deals with building *_compressed* files; blockly_uncompressed.js and the various msg/js/*.js files are not affected by this commit.

I have verified that npm run build && npm run package produces an identical dist/ directory compared to the one produced prior to this and the previous commit.

There are some files in msg/json/ (currently en.json, qqq.json, constants.json and synonyms.json) that are generated by scripts/i18n/js_to_json.py as part of the language file build process - but this only needs to be done when messages.js is updated and and usually requires some manual cleanup, so remove this step from the existing buildLangfiles gulp script and create a separate command ('npm run generate:langfiles') to do this when required.

This is to allow built files to be checked in.

You can now do npm run clean:buildDir, ... clean:releaseDir, or just ... clean, which does both. The release directory is automatically cleaned before packaging commences.

The documented release process is to do npm run recompile, merge the resulting branch to develop, and then do npm run relase, which does not do another build. This process should probably be changed, but for the moment ensure that npm run recompile (as well as npm run package:beta) runs buildTasks.checkinBuilt after each .build to preserve the old procedure.

Don't try to lint built/.

- Do not run npm install. - Do not re-run build that has already been run by a previous test. - Check files in built/ instead repo root. - Fix formatting, styleguide issues.

* Modify scripts/gulpfiles/typings.js to write typings to BUILD_DIR. * Modify tests/scripts/compile_typings.sh to check compilability of resulting output from BUILD_DIR. * Rename checkin script to checkin:built, add a checkin:typings script to do the same for .d.ts files, and a new checkin script to do both. * Have recompile run checkin:typings.

N.B. can't run typings.typings and typings.msgTypings in parallel yet because the latter depends on the existence of an output directory created by the former.

moniika

Re: Test Coverage Section:

I have an alias for untracking build produces using git update-index. I also have listed blockly_uncompressed.js, msg/js/* and msg/json/*. I'm not sure if those should also be considered.

Re: Is there documentation on npm release scripts?

We don't have documentation on our release scripts in core, aside from the jsdoc in the source files. Blockly Samples does have documentation (https://github.com/google/blockly-samples/blob/master/scripts.md) and I think it would be worth it to add something similar to core.

Appengine:

Feel free to ping me about question about the appengine_tasks.js script. I have the most familiarity with that script on the team.
The appengine script copies the checked in files and also uses local files.
The local files used are: version from package.json and playground dependencies in ./node_modules. (some discussion on that here: #4591 (comment))

Misc notes:

I like the addition of a config file, rather than having constants at the top of the scripts.
This may change in the upcoming quarter, but we have needed to update compressed during the quarter, so we might want to document the case when build artifacts should be updated more officially and that npm checkin should be used (assuming that is the solution)

scripts/gulpfiles/appengine_tasks.js

scripts/gulpfiles/config.js

scripts/gulpfiles/build_tasks.js

cpcallen · 2021-07-01T16:04:20Z

I have an alias for untracking build produces using git update-index. I also have listed blockly_uncompressed.js, msg/js/* and msg/json/*. I'm not sure if those should also be considered.

The files in msg/json/ will now only be rebuilt upon manual request via npm run generate:langfiles (see a03cd29).

The files in msg/js/ are now built to built/msg/js/ by npm run build:langfiles (or npm run build or any of the other scripts that invoke that one) and copied back to msg/js/ upon request by npm run checkin (and by some of the release scripts which, run that automatically).

I was wondering about whether the tests depended on msg/js/ and whether they should run against built/msg/js instead, but doing an rm -rf msg/js before running the tests didn't break anything (!) so I don't think there is an issue here (other than a complete absence of test coverage of these .js files).

blockly_uncompressed.js is still being build by build:uncompressed which is run by build, but this this will be dealt with in a separate PR (the file will soon no longer be generated at all).

We don't have documentation on our release scripts in core, aside from the jsdoc in the source files. Blockly Samples does have documentation (https://github.com/google/blockly-samples/blob/master/scripts.md) and I think it would be worth it to add something similar to core.

I can add some. Where should it such an .md file live?

The local files used are: version from package.json and playground dependencies in ./node_modules. (some discussion on that here: Update appengine_tasks to include playground dependencies #4591 (comment))

That is… funky. What are we doing that is not compatible with App Engine's automatic installation of depedencies (in the standard node environment, at least)?

I like the addition of a config file, rather than having constants at the top of the scripts.

One minor issue: that there are a few shell scripts that need the same configuration constant, and I've not thought of a good way to have them read from config.js.

This may change in the upcoming quarter, but we have needed to update compressed during the quarter, so we might want to document the case when build artifacts should be updated more officially and that npm checkin should be used (assuming that is the solution)

Yes. I am hoping someone can help me update the release documentation appropriately.

moniika · 2021-07-01T16:55:43Z

We don't have documentation on our release scripts in core, aside from the jsdoc in the source files. Blockly Samples does have documentation (https://github.com/google/blockly-samples/blob/master/scripts.md) and I think it would be worth it to add something similar to core.

I can add some. Where should it such an .md file live?

I've filed #4983 to track adding the scripts.md. My preference would be at the same level as the package.json.

The local files used are: version from package.json and playground dependencies in ./node_modules. (some discussion on that here: Update appengine_tasks to include playground dependencies #4591 (comment))

That is… funky. What are we doing that is not compatible with App Engine's automatic installation of depedencies (in the standard node environment, at least)?

The project deployed on appengine is not a node.js service (there is no package.json), so the dependencies aren't automatically installed.

The structure on appengine is that the core repository is copied over into a static/ directory and everything that was under appengine/ is moved up to the top level of the directory structure.

If you want to take a better look at what the generated directory and included files for appengine look like, you can comment out the the deployAndClean step of the deployDemos script and look at the deploy directory that is generated.
Keeping in mind there is also a .gitignore used to skip some files and directories (including package.json, script and test directories, etc).

Transcribed (with tweaks) from PR RaspberryPiFoundation#4986.

rachel-fenichel

I have a few small comments, but it looks like Monica has been very thorough.

There is a question about how packaging and releasing ought to work. I initially modified the package script to pull from BUILD_DIR instead of the repository root, but after looking at the recompile and publish tasks I changed this back so as to not change how the release process works. I'm not sure which is preferable:

The advantage of the present arrangement, where we package and release checked-in build products, is that it very easy to see (and test) exactly what will be packaged/released in advance of doing the release.
The disadvantage is that we run the risk of packaging and releasing out-of-date build products. This is especially a risk as none of the automated tests actually test the checked-in build products (with one minor exception: prior to this PR, tests/scripts/compile_typings.sh did actually check the generated .d.ts files in typings/ would compile).
Overall I think it would be better to move to a process that does build, test, package, and release all in a single step, but I do not have much experience in this area to refer to and would appreciate input.

I agree that build->test->package->release is ideal, and also that it doesn't have to be fixed in this PR. I propose that we update the documentation for our day of releasing tasks to include the command needed to rebuild and check in the generated files, which at least reduces the risk of packaging the wrong files.

scripts/gulpfiles/build_tasks.js

The .js.map files generated by buildCompressed, buildBlocks etc. were not being copied back by checkinBuilt.

- Reconfigure package_tasks.js to use BUILD_DIR from config.js (i.e., build/) rather than repository root as source of files to package. - Add check to packageTasks.package to ensure that certain required files exist in BUILD_DIR, to verify that buildTasks.build and typings.typings have been run. - Fix packageUMDBundle to use generated, rather than checked-in version of en.js. - Fix packageDTS to use the generated (rather than checked-in) versions of blockly.d.ts and msg/*.d.ts. - Modify run_all_tests.sh to run packageTasks.package before running node tests, since they depend on it. Previously this was only working because 'npm install' runs the 'prepare' script, which would run the 'package' script - so the code being tested by the node tests was not the current source but whatever precomipled code had previously been checked in. - Remove the 'prepare' script from package.json, since it is no longer needed (and is now broken, since it requires that build and typings have been done first.) Note that no scripts at all are included in the version of package.json that is created in dist/ and subsequently included in the published npms, so this deletion does not affect what happens when the Blockly npm in installed - it only affects what happens when 'npm install' is run after the blockly repo is cloned. - Factor out the actual recompilation steps from releaseTasks.recompile. - Rename releaseTasks.recomple to recompileDevelop, since it deals specifically with the develop branch. (The npm script name is unchanged.) - Ensure that a full recompile and repackage is done before publishing (beta and non-beta) npms.

Per discussion in PR RaspberryPiFoundation#4968, the dirname "build" is widely used for this purpose.

cpcallen · 2021-07-08T16:44:11Z

Because the node tests depend on the contents of RELEASE_DIR (dist/), the only way to actually make sure that they are testing the current original sources is to have the package gulp task take input from BUILD_DIR (renamed build, was built in an earlier version of this branch). See ec14cc3 (and the commit comment thereto) for the gory details, but the most notable changes are:

You have to do npm run build and npm run typings before you can run npm package, and there is now a check to ensure you have. package never uses (or at least should never use) any checked-in copies of files that are generated by build or typings.
npm run recompile does build, typings and checkin (amongst other things).
npm run publish (and publishBeta) now runs build and typings before running package.

This all means it's not strictly necessary to run recompile before publish (but it's probably a good idea to, since it gives the team a chance to review what is about to be published). (For good measure it also runs checkin, so at least there will be evidence after the fact if there were changes in the source not reflected in the checked-in build output.)

@maribethb: Can you have a look at the changes to in release_tasks.js and verify that no changes to the documented release process are required?

@samelhusseini: I'd appreciate you have a close look at this whole PR but especially the changes to package_tasks.js and release_tasks.js.

I believe that this it otherwise ready to merge.

I had forgotten that I needed to change the value of BUILD_DIR in several different places. Added comments warning future editors about this as well as filing issue RaspberryPiFoundation#5007 to track fixing this properly. Despite being misconfigured and therefore failing, the typescript and metadata test scripts were exiting with status 0, indicating successful completion. These have been fixed so they should fail on any error, including misconfiguration.

Fix conflict in package.json.

cpcallen · 2021-07-09T13:18:49Z

Realised I'd not finished the built -> build rename, and (worse) that some tests had been passing even though they should have been failing as result of this oversight, so I've pushed a commit (9e72378) to fix this as well as another (9e72378) merging in the develop branch again so as to resolve a minor merge conflict in package.json.

@rachel-fenichel and @moniika: would you like to review the additional changes since your previous reviews?
@samelhusseini has had a quick look and has said he will do a full review but hasn't had time yet. It would be nice to land this today if at all possible.

rachel-fenichel · 2021-07-09T16:52:14Z

scripts/gulpfiles/package_tasks.js

+    '!typings/blockly.d.ts',  // Exclude checked-in copy of blockly.d.ts.
+    'typings/msg/msg.d.ts',
+  ];
+  const builtSrcs = [


Does this mean that there are actually two versions of typings being included?

There are two sets of typings: handwritten ones, and generated ones. The latter includes (only) blockly.d.ts and the various msg/*.d.ts files; all the rest are handwritten.

Previously they were all pulled from the same place—typings/—but now the generated ones are pulled from build/ instead. But there's still checked-in copies of the generated ones in typings/, and we want to exclude those (because they might be out-of-date if npm run checkin:typings hasn't been run recently) in favour of the generated ones.

So there are two versions, but only one is being included in the npm.

scripts/gulpfiles/release_tasks.js

samelhusseini

I realize this is late, but LGTM.

cpcallen added 14 commits June 21, 2021 19:13

Write build output to built/ instead of repository root

4111073

I have verified that npm run build && npm run package produces an identical dist/ directory compared to the one produced prior to this and the previous commit.

Build msg/js/*.js langfiles in BUILD_DIR

25f073d

Script to copy built files from BUILD_DIR to repository

e2c4a9b

This is to allow built files to be checked in.

Script to clean build directory

961319b

Script to clean release directory

e8eb30f

You can now do npm run clean:buildDir, ... clean:releaseDir, or just ... clean, which does both. The release directory is automatically cleaned before packaging commences.

Fix eslint tests

daab45f

Don't try to lint built/.

Fix metadata tests

96da5ed

- Do not run npm install. - Do not re-run build that has already been run by a previous test. - Check files in built/ instead repo root. - Fix formatting, styleguide issues.

Merge branch 'develop' into build-elsewhere

1aa35ef

Do trivial gulp tasks in parallel where possible

5410ba6

N.B. can't run typings.typings and typings.msgTypings in parallel yet because the latter depends on the existence of an output directory created by the former.

cpcallen requested review from rachel-fenichel and samelhusseini June 30, 2021 16:54

cpcallen added the component: build process label Jun 30, 2021

moniika reviewed Jul 1, 2021

View reviewed changes

scripts/gulpfiles/appengine_tasks.js Show resolved Hide resolved

scripts/gulpfiles/config.js Outdated Show resolved Hide resolved

scripts/gulpfiles/build_tasks.js Show resolved Hide resolved

moniika mentioned this pull request Jul 1, 2021

Add scripts documentation #4983

Open

3 tasks

Update metadata sizes for Q2 2021 release

c265eb8

Transcribed (with tweaks) from PR RaspberryPiFoundation#4986.

cpcallen mentioned this pull request Jul 2, 2021

Update metadata sizes for Q2 2021 release #4986

Closed

3 tasks

rachel-fenichel approved these changes Jul 3, 2021

View reviewed changes

scripts/gulpfiles/build_tasks.js Outdated Show resolved Hide resolved

scripts/gulpfiles/build_tasks.js Show resolved Hide resolved

cpcallen added 4 commits July 7, 2021 15:36

Also check in sourcemaps

e6929de

The .js.map files generated by buildCompressed, buildBlocks etc. were not being copied back by checkinBuilt.

Rename BUILD_DIR to "build" (was "built")

311b2c1

Per discussion in PR RaspberryPiFoundation#4968, the dirname "build" is widely used for this purpose.

Fix comment TODO formatting.

1e4fa80

cpcallen marked this pull request as ready for review July 8, 2021 16:30

cpcallen mentioned this pull request Jul 8, 2021

Added script and workflow to automatically update build artifact sizes in check_metadata.sh #4997

Closed

3 tasks

cpcallen requested a review from a team as a code owner July 9, 2021 13:07

Merge branch 'develop' into build-elsewhere

ffd0252

Fix conflict in package.json.

cpcallen mentioned this pull request Jul 9, 2021

Local http server for testing in uncompiled mode #5008

Merged

3 tasks

rachel-fenichel reviewed Jul 9, 2021

View reviewed changes

scripts/gulpfiles/release_tasks.js Show resolved Hide resolved

Rename internal recompile function to rebuildAll

c45929d

rachel-fenichel approved these changes Jul 9, 2021

View reviewed changes

cpcallen merged commit 899493e into RaspberryPiFoundation:develop Jul 9, 2021

cpcallen deleted the build-elsewhere branch July 9, 2021 22:16

samelhusseini reviewed Jul 9, 2021

View reviewed changes

Build in a temporary directory #4968

Build in a temporary directory #4968

Uh oh!

Conversation

cpcallen commented Jun 30, 2021

The basics

The details

Proposed Changes

Behavior Before Change

Behavior After Change

Reason for Changes

Test Coverage

Documentation

Additional Information

Release process

Appengine

Uh oh!

moniika left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpcallen commented Jul 1, 2021

Uh oh!

moniika commented Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rachel-fenichel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cpcallen commented Jul 8, 2021

Uh oh!

cpcallen commented Jul 9, 2021

Uh oh!

rachel-fenichel Jul 9, 2021

Choose a reason for hiding this comment

Uh oh!

cpcallen Jul 9, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

samelhusseini left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

moniika commented Jul 1, 2021 •

edited

Loading