Skip to content

Commit

Permalink
build: Restore --version flag support, update new flag name
Browse files Browse the repository at this point in the history 
Revise several changes made in PR ampproject#27196:
- 'number' was misspelled several times in TESTING.md and is not a
  necessary word anyways. Remove 'number' from descriptions of flag.
- Flag --version has been supported since 2016 (ampproject#3702). It was
  inappropriate to simply remove support for the flag, when it can be
  left in-place without any harm to the build system.
- `rtv_version` is not a good choice of name, it reads like "runtime
  version version" and runtime versions differ from release versions
  (runtime version includes config number and release version number).
  Opt for `release_version` which lines up with the version listed on
  the release page https://github.com/ampproject/amphtml/releases.
  • Loading branch information
@mdmower
mdmower committed Mar 14, 2020
1 parent 030afb4 commit 6ac141d
Show file tree
Hide file tree
Showing 5 changed files with 19 additions and 10 deletions.
6 changes: 4 additions & 2 deletions build-system/compile/internal-version.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@ const argv = require('minimist')(process.argv.slice(2));
const {gitCommitFormattedTime} = require('../common/git');

function getVersion() {
if (argv.rtv_version) {
return String(argv.rtv_version);
// Flag --version has been supported since 2016, so is still supported for
// backwards compatibility. --release_version is the documented flag.
if (argv.release_version || argv.version) {
return String(argv.release_version || argv.version);
} else {
// Generate a consistent version number by using the commit* time of the
// latest commit on the active branch as the twelve digits. The last,
Expand Down
9 changes: 6 additions & 3 deletions build-system/tasks/build.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,9 @@ build.flags = {
noextensions: ' Builds with no extensions.',
core_runtime_only: ' Builds only the core runtime.',
coverage: ' Adds code coverage instrumentation to JS files using istanbul.',
rtv_version: ' Overrides the version number written to AMP_CONFIG',
release_version: ' Overrides the version written to AMP_CONFIG',
};
build.undocumented_flags = ['version'];

watch.description = 'Watches for changes in files, re-builds when detected';
watch.flags = {
Expand All @@ -143,8 +144,9 @@ watch.flags = {
' Watches and builds only the extensions from the listed AMP(s).',
noextensions: ' Watches and builds with no extensions.',
core_runtime_only: ' Watches and builds only the core runtime.',
rtv_version: ' Overrides the version number written to AMP_CONFIG',
release_version: ' Overrides the version written to AMP_CONFIG',
};
watch.undocumented_flags = ['version'];

defaultTask.description =
'Starts the dev server and lazily builds JS and extensions when requested';
Expand All @@ -154,5 +156,6 @@ defaultTask.flags = {
extensions_from:
' Watches and builds only the extensions from the listed AMP(s).',
noextensions: ' Watches and builds with no extensions.',
rtv_version: ' Overrides the version number written to AMP_CONFIG',
release_version: ' Overrides the version written to AMP_CONFIG',
};
defaultTask.undocumented_flags = ['version'];
3 changes: 2 additions & 1 deletion build-system/tasks/dist.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -452,5 +452,6 @@ dist.flags = {
" Doesn't use nailgun to invoke closure compiler (much slower)",
type: ' Points sourcemap to fetch files from the correct GitHub tag',
esm: ' Does not transpile down to ES5',
rtv_version: ' Override the version number written to AMP_CONFIG',
release_version: ' Override the version written to AMP_CONFIG',
};
dist.undocumented_flags = ['version'];
8 changes: 4 additions & 4 deletions contributing/TESTING.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ Before running these commands, make sure you have Node.js, yarn, and Gulp instal
| `gulp --extensions=minimal_set` | Runs "watch" and "serve", after building the extensions needed to load `article.amp.html`. |
| `gulp --extensions_from=examples/foo.amp.html` | Runs "watch" and "serve", after building only extensions from the listed examples. |
| `gulp --noextensions` | Runs "watch" and "serve" without building any extensions. |
| `gulp --rtv_version=<rtv_version>` | Runs "watch" and "serve". Overrides the version nubmer written to the AMP_CONFIG. |
| `gulp --release_version=<release_version>` | Runs "watch" and "serve". Overrides the version written to the AMP_CONFIG. |
| `gulp dist` | Builds production binaries and applies AMP_CONFIG to runtime files. |
| `gulp dist --noconfig` | Builds production binaries without applying AMP_CONFIG to runtime files. |
| `gulp dist --extensions=amp-foo,amp-bar` | Builds production binaries, with only the listed extensions. |
Expand All @@ -58,7 +58,7 @@ Before running these commands, make sure you have Node.js, yarn, and Gulp instal
| `gulp dist --core_runtime_only` | Builds production binary for just the core runtime. |
| `gulp dist --fortesting` | Builds production binaries for local testing. (Allows use cases like ads, tweets, etc. to work with minified sources. Overrides `TESTING_HOST` if specified. Uses the production `AMP_CONFIG` by default.) |
| `gulp dist --fortesting --config=<config>` | Builds production binaries for local testing, with the specified `AMP_CONFIG`. `config` can be `prod` or `canary`. (Defaults to `prod`.) |
| `gulp dist --rtv_version=<rtv_version>` | Builds production binaries and overrides the version nubmer written to the AMP_CONFIG. |
| `gulp dist --release_version=<release_version>` | Builds production binaries and overrides the version written to the AMP_CONFIG. |
| `gulp lint` | Validates JS files against the ESLint linter. |
| `gulp lint --watch` | Watches for changes in files, and validates against the ESLint linter. |
| `gulp lint --fix` | Fixes simple lint warnings/errors automatically. |
Expand All @@ -75,7 +75,7 @@ Before running these commands, make sure you have Node.js, yarn, and Gulp instal
| `gulp build --noextensions` | Builds the AMP library with no extensions. |
| `gulp build --core_runtime_only` | Builds only the core runtime of the AMP library. |
| `gulp build --fortesting` | Builds the AMP library and sets the `test` field in `AMP_CONFIG` to `true`. |
| `gulp build --rtv_version=<rtv_version>` | Builds the AMP library with the specified version number. |
| `gulp build --release_version=<release_version>` | Builds the AMP library with the specified version. |
| `gulp check-links --files=<files-path-glob>` | Reports dead links in `.md` files. |
| `gulp check-links --local_changes` | Reports dead links in `.md` files changed in the local branch. |
| `gulp clean` | Removes build output. |
Expand All @@ -87,7 +87,7 @@ Before running these commands, make sure you have Node.js, yarn, and Gulp instal
| `gulp watch --extensions_from=examples/foo.amp.html` | Watches for changes in files, re-builds only the extensions needed to load the listed examples. |
| `gulp watch --noextensions` | Watches for changes in files, re-builds with no extensions. |
| `gulp watch --core_runtime_only` | Watches for changes in the core runtime, re-builds. |
| `gulp watch --rtv_version=<rtv_version>` | Watches for changes in files, rebuilds. Overrides the version nubmer written to the AMP_CONFIG. |
| `gulp watch --release_version=<release_version>` | Watches for changes in files, rebuilds. Overrides the version written to the AMP_CONFIG. |
| `gulp pr-check` | Runs all the Travis CI checks locally. |
| `gulp pr-check --nobuild` | Runs all the Travis CI checks locally, but skips the `gulp build` step. |
| `gulp pr-check --files=<test-files-path-glob>` | Runs all the Travis CI checks locally, and restricts tests to the files provided. |
Expand Down
3 changes: 3 additions & 0 deletions gulpfile.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,9 @@ function checkFlags(name, taskFunc) {
return; // This isn't the task being run.
}
const validFlags = taskFunc.flags ? Object.keys(taskFunc.flags) : [];
if (Array.isArray(taskFunc.undocumented_flags)) {
Array.prototype.push.apply(validFlags, taskFunc.undocumented_flags);
}
const usedFlags = Object.keys(argv).slice(1); // Skip the '_' argument
const invalidFlags = [];
usedFlags.forEach(flag => {
Expand Down

0 comments on commit 6ac141d

Please sign in to comment.