chore(build): Remove build products from the Blockly repository #6475

cpcallen · 2022-10-04T15:59:06Z

The basics

I branched from develop
My pull request is against develop
My code follows the style guide
I ran npm run format and npm run lint

The details

Resolves

Resolves #6403, #6159

Proposed Changes

Divide gulp targets into three kinds: main sequence, manually invokable, and script-only. The first two categories automatically invoke their prerequisites.
- Give some of the affected gulp targets shorter and more memorable names that could (depending on feedback on this PR) become their npm script names.
- Have the package task invoke the cleanBuildDir (as well as cleanPackageDir) and build tasks.
- Remove the checkBuildDir task as it is now redundant since a fresh build is done every time.
- Have npm test run just the package target (with debug flags), since that runs the the build target automatically.
Have any npm script that has external effects (e.g. publishing an npm package, pushing a new version to App Engine, or updating GitHub Pages) start by running npm ci to ensure that all dependencies are up-to-date with respect to package-lock.json.
- This is done by npm and not a gulp script because gulp itself might need updating. (So might npm, but that is less likely to make any difference to what gets published/pushed.)
Have Closure Compiler write its output directly to RELEASE_DIR (dist/) since it comes out already UMD-wrapped.
Have build:langfiles write to build/msg/ instead of build/msg/js, for consistency with ultimate destination for wrapped files (dist/msg/).
Modify tests/bootstrap.js to, when loading in compressed mode, use the freshly-built dist/blockly_compressed.js instead of the probably-stale checked in version in the repository root (and similarly for all the other chunks).
Modify playgrounds to use build/msg/en.js instead of msg/messages.js.
- All playgrounds load messages in a way that does not depend on the UMD wrapper.
Modify demos, appengine deployment and GitHub pages release tasks to use files in build/ and/or dist/.
Remove all checked-in build products.

Behaviour Before Change

It was fairly easy to test or release outdated files by accident.

Behaviour After Change

It should be more or less impossible to inadvertently publish an out-of-date file. Playgrounds etc. will only load the most recently-built versions of their Blockly dependencies.

Reason for Changes

It is not generally considered good practice to check build products into a source code repository; there are other better means, such as npm packages and GitHub releases, to publish released "binaries".

Documentation

There will need to be updates made to the release process documentation.

Additional Information

* Divide gulp targets into three kinds: main sequence, manually invokable, and script-only. The first two categories automatically invoke their prerequisites. * Give (most of) the affected gulp targets shorter and more memorable names that could become their npm script names in future.

Have the package task invoke the cleanBuildDir (as well as cleanPackageDir) and build tasks. Remove the checkBuildDir task as it is now redundant since a fresh build is done every time.

…ites Turns out they don't have any, so this commit just classifies their gulp targets according to the established scheme.

In this case prepareDeployDir will eventually depend on package but does not for now.

Have any npm script that have external effects (e.g. publishing an npm package, pushing a new version to appengine, or updating GitHub Pages) start by running npm ci to ensure that all dependencies are up-to-date with respect to package-lock.json. (This is done by npm and not a gulp script because gulp itself might need updating. So might npm, but that is less likely to make any difference to what gets published/pushed.)

Have the tests just run the package target (with debug flags) since that runs the the build target automatically.

Since they are already UMD-wrapped, have Closure Compiler write output chunks directly to RELEASE_DIR, i.e. dist/.

Use the freshly-built build/*_compresssed.js files when bootstrapping in compressed mode, rather than using the checked-in files in the repository root. This helps ensure that compressed and uncompressed mode will be testing (as closely as possible) the same code. Obsoletes google#6218 (though the issues discussed there have not actually yet been addressed in this branch).

Write the results of create_messages.py to build/msg instead of build/msg/js.

This has no direct effect but fixes a long-standing misdesign where we are testing against the input to, rather than the output of, the language file processing pipeline.

Use the freshly-built dist/*_compresssed.js and build/msg/* files rather than using the checked-in files in the repository root. This helps ensure that these demos are using the most recent version of Blockly (even in the develop branch).

Modify the prepareDemos task as follows: * Use the git index instead of HEAD, so that most local changes will be applied (without copying whatever .gitignored cruft might be in the local directory).  * Run clean and build and then copy build/msg and dist/*_compressed.js* to the deploy directory. This fixes the problem created by the previous commit, wherein the demos relied on built files that were not being deployed to appengine.

Modify the updateGithubPages task to run clean and build and then git add build/msg dist/*_compressed.js*, so that they will be included in the deployed pages. This fixes the problem created by the previous^2 commit, wherein the demos relied on built files that were not being deployed to GitHub Pages.

package.json

scripts/gulpfiles/build_tasks.js

scripts/gulpfiles/git_tasks.js

package.json

tests/run_all_tests.sh

scripts/gulpfiles/package_tasks.js

demos/mobile/android/app/build.gradle

scripts/gulpfiles/appengine_tasks.js

scripts/gulpfiles/git_tasks.js

BeksOmega · 2022-10-04T16:39:17Z

package.json

@@ -31,7 +31,6 @@
    "clean": "gulp clean",
    "clean:build": "gulp cleanBuildDir",
    "clean:release": "gulp cleanReleaseDir",
-    "checkin": "gulp checkin",


Could we remove some more of these? e.g. now that everything properly calls prereqs, is there any reqson to call clean:build manually?

I guess I like to be able to manually clean because when I started University we got a default disk quota of 1 MiB and so one really wanted to be able to get rid of intermediate files when one was finished building something…

But yes; I want to remove a bunch of these. Just not sure which ones are actually being used regularly by our teammates. I do think that e.g. clean is sufficient, and clean:builddir and clean:releasedir are excessively precise. Maybe I was just a bit drunk on command:subcommand syntax when I added them…

Remove *_compressed.js* and msg/js/* from the blockly repository. Also remove the now-obsolete checkinBuilt gulp task.

Files that have had manual merge conflict resolution performed: - gulpfile.js Files where a merge conflic was resolved by deleting the file, because it had previously been deleted in no-checkin-built: - *_compresed.js* - msg/js/*.js - tests/run_all_tests.sh Changes made (in no-checkin-built) to the following files that have been deleted or obsoleted will need to be dealt with manually; this will be done in a separate commit for clarity: - tests/run_all_tests.sh - tests/scripts/check_metadata.sh - tests/scripts/run_generators.sh

Apply changes made to run_all_tests.sh and check_metadata.sh to the corresponding parts of their JS replacements in test_tasks.js.

- Remove `clean:builddir` and `clean:releasedir` - `clean` is sufficient. - Remove duplicate `require` from `appengine_tasks.js`.

Since scripts that run build tasks now automatically run their prerequisite tasks, the previous naming scheme of task `build` running all the `build:subtask`s no longe really makes very much sense. Additionally, following a chat discussion, there seems to be a rough consensus to use "messages" to refer to the .json input files, and "langfiles" to the generated .js output files. Consequently, simplify npm script names by renaming as follows: - "generate:langfiles" -> "messages" - "build:langfiles" -> "langfiles" - "build:js" -> "tsc" - "build:deps" -> "deps" - "build:compiled" -> "minify" - "build:compressed": delete this synonym for "build:compiled", ("minify" was chosen as agnostic to Closure Compiler vs. WebPack.)

To reduce potential confusion/frustration, restore the previous npm scripts but have them display a deprecation notice instead (note that npm prints the script contents before running it, so echo is not needed).

cpcallen · 2022-11-02T14:13:04Z

I believe I'm now done messing around with this. Apologies for the slightly convoluted history; I changed my mind about teh contents of the last couple of commits several times…

BeksOmega

LGTM! One last thing: could you add a comment smewhere explaining the difference between messages and langfiles? Maybe in gulpfile.js?

The bash script tests/run_all_tests.js was deleted in PR google#6431, where it was replaced by `scripts/gulpfiles/test_tasks.js`, but it was inadvertently resurrected by PR google#6475, apparently due to an error when manually resolving conflicts for merge commit 00676ed. Begone, undead script!

The bash script tests/run_all_tests.sh was deleted in PR #6431, where it was replaced by scripts/gulpfiles/test_tasks.js, but it was inadvertently resurrected by PR #6475, apparently due to an error when manually resolving conflicts for merge commit 00676ed. Begone, undead script!

This change had already been made (by PR google#6475 amongst other) to most of the tests, but there were a couple of stragglers that still mentioned messages.js. Fixes google#6824.

This change had already been made (by PR #6475 amongst other) to most of the tests, but there were a couple of stragglers that still mentioned messages.js. Fixes #6824.

cpcallen added 14 commits September 27, 2022 13:55

feat(build): Make package tasks invoke their prerequisites

1d462f2

Have the package task invoke the cleanBuildDir (as well as cleanPackageDir) and build tasks. Remove the checkBuildDir task as it is now redundant since a fresh build is done every time.

feat(build): Make git tasks invoke their prerequisites

4a7bf1e

feat(build): Make cleanup, license [sic] tasks invoke their prerequis…

e602358

…ites Turns out they don't have any, so this commit just classifies their gulp targets according to the established scheme.

feat(build): Make appengine tasks invoke their prerequisites

6f7532b

In this case prepareDeployDir will eventually depend on package but does not for now.

chore(build): have tests use package target

16c7165

Have the tests just run the package target (with debug flags) since that runs the the build target automatically.

feat(tests): Write Closure Compiler output directly to dist/

4d150ff

Since they are already UMD-wrapped, have Closure Compiler write output chunks directly to RELEASE_DIR, i.e. dist/.

chore(build): Write intermediate langfiles to build/msg

c99385f

Write the results of create_messages.py to build/msg instead of build/msg/js.

fix(build): Use build/msg/en.js instead of msg/messages.js in tests

3f2ff19

This has no direct effect but fixes a long-standing misdesign where we are testing against the input to, rather than the output of, the language file processing pipeline.

feat(demos): Use freshly-built files

8d06fe7

Use the freshly-built dist/*_compresssed.js and build/msg/* files rather than using the checked-in files in the repository root. This helps ensure that these demos are using the most recent version of Blockly (even in the develop branch).

cpcallen requested a review from BeksOmega October 4, 2022 15:59

cpcallen assigned BeksOmega Oct 4, 2022

BeksOmega reviewed Oct 4, 2022

View reviewed changes

chore(build): Remove build products from repository

ced4b1d

Remove *_compressed.js* and msg/js/* from the blockly repository. Also remove the now-obsolete checkinBuilt gulp task.

BeksOmega mentioned this pull request Oct 5, 2022

test: add shared procedures playground #6485

Merged

4 tasks

cpcallen force-pushed the no-checkin-built branch from f9a3383 to ced4b1d Compare October 7, 2022 12:23

cpcallen added 2 commits October 28, 2022 23:15

chore(build): Apply relevant changes to test_tasks.js

234ccbe

Apply changes made to run_all_tests.sh and check_metadata.sh to the corresponding parts of their JS replacements in test_tasks.js.

cpcallen added a commit to cpcallen/blockly that referenced this pull request Nov 1, 2022

chores(build): Make updates suggested in PR google#6475

0c2e737

- Remove `clean:builddir` and `clean:releasedir` - `clean` is sufficient. - Remove duplicate `require` from `appengine_tasks.js`.

cpcallen added component: build process PR: chore General chores (dependencies, typos, etc) labels Nov 1, 2022

cpcallen changed the title ~~chore!(build): Remove build products from the Blockly repository~~ chore(build): Remove build products from the Blockly repository Nov 1, 2022

github-actions bot removed the PR: chore General chores (dependencies, typos, etc) label Nov 1, 2022

chore(build): Make updates suggested in PR google#6475

d36a1a7

- Remove `clean:builddir` and `clean:releasedir` - `clean` is sufficient. - Remove duplicate `require` from `appengine_tasks.js`.

github-actions bot added the PR: chore General chores (dependencies, typos, etc) label Nov 1, 2022

cpcallen force-pushed the no-checkin-built branch from 0c2e737 to d36a1a7 Compare November 1, 2022 16:48

cpcallen marked this pull request as ready for review November 1, 2022 16:48

cpcallen requested a review from a team as a code owner November 1, 2022 16:48

cpcallen added 2 commits November 2, 2022 00:14

chores(build): Add deprecation notice for old scripts

fbe2b82

To reduce potential confusion/frustration, restore the previous npm scripts but have them display a deprecation notice instead (note that npm prints the script contents before running it, so echo is not needed).

cpcallen force-pushed the no-checkin-built branch from 0182616 to fbe2b82 Compare November 2, 2022 14:08

BeksOmega approved these changes Nov 2, 2022

View reviewed changes

docs(build): Add comments distinguishing 'messages' from 'langfiles'

ec8b4d0

cpcallen merged commit 52a0d52 into google:develop Nov 3, 2022

cpcallen deleted the no-checkin-built branch November 3, 2022 13:15

cpcallen mentioned this pull request Nov 3, 2022

chore(tests): Use freshly-build files in compressed mode. #6218

Closed

4 tasks

cpcallen mentioned this pull request Nov 17, 2022

chore(build): Re-delete tests/run_all_tests.sh #6636

Merged

4 tasks

cpcallen mentioned this pull request Nov 28, 2022

Cannot find viable example of use of "blockly_compressed.js" with current version #6651

Closed

cpcallen mentioned this pull request Dec 7, 2022

fix(appengine): Restore build products to previous location #6687

Merged

4 tasks

github-actions bot added PR: chore General chores (dependencies, typos, etc) and removed PR: chore General chores (dependencies, typos, etc) labels Feb 6, 2023

cpcallen mentioned this pull request Feb 6, 2023

Have the npm run package script run the build #6159

Closed

cpcallen mentioned this pull request Feb 6, 2023

fix(tests): Use build/msg/en.js instead of msg/messages.js #6825

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(build): Remove build products from the Blockly repository #6475

chore(build): Remove build products from the Blockly repository #6475

cpcallen commented Oct 4, 2022 •

edited

Loading

BeksOmega Oct 4, 2022

cpcallen Nov 1, 2022

cpcallen commented Nov 2, 2022

BeksOmega left a comment

chore(build): Remove build products from the Blockly repository #6475

chore(build): Remove build products from the Blockly repository #6475

Conversation

cpcallen commented Oct 4, 2022 • edited Loading

The basics

The details

Resolves

Proposed Changes

Behaviour Before Change

Behaviour After Change

Reason for Changes

Documentation

Additional Information

BeksOmega Oct 4, 2022

Choose a reason for hiding this comment

cpcallen Nov 1, 2022

Choose a reason for hiding this comment

cpcallen commented Nov 2, 2022

BeksOmega left a comment

Choose a reason for hiding this comment

cpcallen commented Oct 4, 2022 •

edited

Loading