build: Update version flags

[mdmower]

Document flag custom_version_mark.

Revise some changes made in PR #27196:

• 'number' was misspelled in TESTING.md and is not a necessary word anyways. Remove 'number' from descriptions of flag.
• Rename rtv_version flag to version_override. "rtv version" reads like "runtime version version" and runtime versions differ from release versions (prefixed by config number).

[micajuine-ho]

@mdmower Thanks for these changes!

I agree with the points you made above and the revisions look good to me.

To @rsimha for the gulpfile.js changes.

[amp-owners-bot]

Hey @danielrozenberg, these files were changed:

build-system/compile/internal-version.js

[rsimha]

@mdmower Thanks for your attention to detail, and for improving our toolchain. I've added some comments below to further improve things. Let me know what you think.

• 'number' was misspelled several times in TESTING.md and is not a
  necessary word anyways. Remove 'number' from descriptions of flag.

Good catch on the misspelling. See comment below on why we use "number".

• Flag --version has been supported since 2016 (use the version flag if provided #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.

The italicized part of your statement isn't strictly true. See my comment below about reserved flags in the gulp CLI.

• 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 ampproject/amphtml/releases.

I agree with your point about "runtime version version" 😃 I don't think we should use release_version either, since we don't actually use that term in our open-source code or our internal release code. Meanwhile, the getVersion function in build-system/compile/internal-version.js uses the term "version number" (it comes from our internal release system, and is used to distinguish from other non-numeric strings).

WDYT about renaming the flag to --version_number or --version_override? (Pick the one that seems more intuitive to you.)

[mdmower]

The italicized part of your statement isn't strictly true. See my comment below about reserved flags in the gulp CLI.

The build system isn't harmed by usage of the --version=123 flag. I agree the command line can returned unexpected results if used as --version 123, but that's unrelated to AMP. So, I'm totally on-board with creating a new flag name; I just don't want to remove support for --version=123.

Meanwhile, the getVersion function in build-system/compile/internal-version.js uses the term "version number"

I didn't explain, but part of the reason I'm avoiding "version number" is that I'm not sure version must strictly be a number. I've been meaning to do some testing with non numeric versions so that I give accurate instructions in my self-hosted AMP framework guide. If it is possible to use version 1.2a, then I don't want to forbid publishers from using their preferred versioning system.

WDYT about renaming the flag to --version_number or --version_override? (Pick the one that seems more intuitive to you.)

I suppose I would lean towards --version_override in this case.

[rsimha]

The build system isn't harmed by usage of the --version=123 flag. I agree the command line can returned unexpected results if used as --version 123, but that's unrelated to AMP. So, I'm totally on-board with creating a new flag name; I just don't want to remove support for --version=123.

--version has always been an undocumented flag that has required a specific invocation style (--flag=value works, --flag value doesn't) for at least two years (since #15092, which added gulp-cli). In addition, the use of invalid flags has been forbidden by the build system for the past month (since #26814).

My main objection in this PR is to the changes in gulpfile.js (adding taskFunc.undocumented_flags). I don't think the build system ought to continue supporting undocumented flags, especially if they have since become reserved flags for node or gulp.

If there are manual workflows that used to rely on --version, can you explain why they can't start using --version_override from here on?

I didn't explain, but part of the reason I'm avoiding "version number" is that I'm not sure version must strictly be a number. I've been meaning to do some testing with non numeric versions so that I give accurate instructions in my self-hosted AMP framework guide. If it is possible to use version 1.2a, then I don't want to forbid publishers from using their preferred versioning system.

Thanks for the context. This scenario should be supported by gulp build --version_override "1.2a", correct?

I suppose I would lean towards --version_override in this case.

SGTM.

[mdmower]

If there are manual workflows that used to rely on --version, can you explain why they can't start using --version_override from here on?

Of course users can migrate their workflows to the updated flag. The issue I have with PR #26814 and #27196 is the lack of communication and time allotted to deprecating one flag in favor of another. --version was killed without any notice to the community.

[rsimha]

Of course users can migrate their workflows to the updated flag. The issue I have with PR #26814 and #27196 is the lack of communication and time allotted to deprecating one flag in favor of another. --version was killed without any notice to the community.

Thanks for your thoughts, @mdmower. A few points in response:

• 🏗 Ensure valid flag usage for gulp tasks #26814 did not remove or deprecate any flags, all it did was add a check for the use of invalid flags, which was a commonly reported problem given the number of workflows supported by our build system. It was immediately followed up by multiple clean-up PRs that documented all valid flags.
• 🏗 Apply rtv_version flag to dist and build tasks #27196 was merged to unblock the existing workflow around overriding versions, which had been broken for most developers for at least the past month. The flag rename was a side effect of --version being reserved by gulp / node.
• The workflow we've used to change documented flags / tasks has been to remove them a month after they have been deprecated. (For recent examples, see Remove the --eager_build flag for the default gulp task #24423 and 🏗 Print warning that gulp test will be deprecated soon #22772.) In this case though, the problem lay in the fact that the workflows in question were relying on an undocumented flag.

Finally, I do accept your higher level point, and agree that we can do better with communicating changes. Will keep this in mind in future.

[mdmower]

Finally, I do accept your higher level point, and agree that we can do better with communicating changes. Will keep this in mind in future.

Thanks for acknowledging.

I don't think the build system ought to continue supporting undocumented flags...

In the same getVersion() function where the --version flag used to be supported, there is another undocumented flag: --custom_version_mark. What will be its fate?

[rsimha]

In the same getVersion() function where the --version flag used to be supported, there is another undocumented flag: --custom_version_mark. What will be its fate?

Good catch. Since it isn’t reserved, and will continue to work once documented, let’s document it. (Are you okay with adding it to this PR?)

[rsimha]

LGTM!