Update the pipeline to generate addon build versions that are compatible with MV3

[gabrielBusta]

Fixes #185

Prior art on #197

Documentation https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/version#version_format

I think PR decisions are busted because the branch name has a plus(?)

[gabrielBusta]

The decisions are working again...

[gabrielBusta]

Nvm the other PR is busted, the cache was making it seem like it was fine with the + in the branch name

[gabrielBusta]

We will need commits on the addon repositories to drop the .patch part of the versions from their manifest.json package.json files. Like this: mozilla-extensions/balrog-dryrun@d311b29

[gabrielBusta]

These are archived and can't be updated to use a different versioning scheme:

• addons-restricted-domains
• secure-proxy

We should remove them from the pipeline.

[bhearsum]

These are archived and can't be updated to use a different versioning scheme:

* [addons-restricted-domains](https://github.com/mozilla-extensions/addons-restricted-domains)

* [secure-proxy](https://github.com/mozilla-extensions/secure-proxy)

We should remove them from the pipeline.

Makes sense to me.

[gabrielBusta]

note: it's actually package.json

[willdurand]

is this documented anywhere? Maybe we could add a new section about extension versions in https://github.com/mozilla-extensions/xpi-manifest/blob/main/docs/adding-a-new-xpi.md#creating-the-official-repo?

[gabrielBusta]

This is actually confusing. Is the version Firefox cares about the one in the manifest? This bit of documentation from the migration guide seems to say so.

Extension version in the manifest

The format of the top-level manifest.json version key in Firefox has evolved and became simpler: letters and other previously allowed symbols are no longer accepted. The value must be a string with 1 to 4 numbers separated by dots (e.g., 1.2.3.4). Each number can have up to 9 digits and leading zeros before another digit are not allowed (e.g., 2.01 is forbidden, but 0.2, 2.0.1, and 2.1 are allowed)

[gabrielBusta]

Never mind. I see what is going on. The code reads the version from package dot json appends a build id - then updates the version in both package dot json and manifest dot json to the new build id version.

[willdurand]

@ahal (Andrew Halberstadt) marked this pull request as draft last week

Can we move forward with this patch please?

[gabrielBusta]

The two digit version is causing some issues with build tools. For example this npm-run-all package is throwing an "invalid version" error because the version if not a "semantic" version.

https://github.com/mozilla-extensions/nimbus-devtools/pull/8/checks?check_run_id=21624968034

:/

[gabrielBusta]

If we specify this non semantic versions in package.json people might run into issues when trying to use certain npm commands and packages :(

[willdurand]

The spec only mentions that the version string must be parseable by some node library: https://docs.npmjs.com/cli/v10/configuring-npm/package-json#version - and looking at it, it feels like <major>.<minor> is a valid string.

[willdurand]

I think the issue with npm-run-all comes from https://github.com/npm/normalize-package-data/blob/d3bfe732968229ecaf4daec3ba5c2345ff8030fb/lib/fixer.js#L205 (via https://github.com/mysticatea/npm-run-all/blob/bf91f94ce597aa61da37d2e4208ce8c48bc86673/lib/read-package-json.js#L14C25-L14C34 and https://github.com/sindresorhus/read-pkg/blob/a3732e2307e6a47d22600fa769debfa557b3690a/index.js#L16), which calls this function: https://github.com/npm/node-semver/blob/ac9b35769ab0ddfefd5a3af4a3ecaf3da2012352/functions/valid.js

The issue is that - for some reason - npm itself accepts <major>.<minor> in the manifest but the same library used via npm-run-all doesn't "see" it as a valid semantic version. Bah.

[gabrielBusta]

Ah, it feels like <major>.<minor> is a valid string.

But, if the spec's requirement is that:

Version must be parseable by node-semver

Then, <major>.<minor> doesn't follow the spec.

This code:

const parse = require("semver/functions/parse");

function check(version) {
  const semver = parse(version);
  if (!semver) {
    console.error(`"${version}" is not parseable by node-semver.`);
  } else {
    console.log(`"${version}" is parseable by node-semver.`);
    console.log(semver);
  }
}

check("1.2.3");
check("1.2");

Ouputs:

Version "1.2.3" is parseable by node-semver.
SemVer {
  options: {},
  loose: false,
  includePrerelease: false,
  raw: '1.2.3',
  major: 1,
  minor: 2,
  patch: 3,
  prerelease: [],
  build: [],
  version: '1.2.3'
}
Version "1.2" is not parseable by node-semver.

[gabrielBusta]

Pretty much the same as valid.

[gabrielBusta]

Thanks for digging into it @willdurand

We should let the extension repositories specify versions in their package.json that are parsable by this library.
There might be more build tooling that relies on it (even if the extensions are not usually published to npm registries.)

We could require the extensions repositories to specify a unique semantic version in their package.json file for each release. Or we could start reading the version from manifest.json files? But, I'd have to look more into it, there might be a reason we are using the package.json file. Mm, or maybe we could use the 4th element as a timestamp (1.2.3.<timestamp>)

[willdurand]

This looks good to me. Should we revert the changes we've made to some of the repos (when we dropped the third version part in the package.json files?)

[gabrielBusta]

This looks good to me. Should we revert the changes we've made to some of the repos (when we dropped the third version part in the package.json files?)

IIRC those repos are not using tooling that requires the 3 parts. So, we don't have to (and it's more transparent to have a two part version.) The timestamp is just appended and nothing is over-ridden. The repos can always add a third part/0 if they need the escape hatch to satisfy tooling relying on node-semver.

[willdurand]

Seeing the check against the buildid_version right before, this new check would mean we somehow failed to make a compliant version string, right?

[gabrielBusta]

Yeah, that's right. This is checking the generated versions in the manifest(s) to make sure they are compliant. It is most likely that this check will fail because of the manually crafted part of the string. But the in-container docker script/executable is not unit tested. So I added this as a sanity check/sign-post.

[willdurand]

I expect the fingerprint extension to fail even after that because we still need to allow MV3 extensions explicitly but otherwise that looks good, I think.

[gabrielBusta]

I expect the fingerprint extension to fail even after that because we still need to allow MV3 extensions explicitly but otherwise that looks good, I think.

I see! I opened an issue RE the addons-linter

[willdurand]

Greeeen, finally!

[bhearsum]

Shouldn't this be (1,2,3,4)? From the docs:

The version string consists of 1 to 4 numbers separated by dots, for example, 1.2.3.4. Non-zero numbers must not include a leading zero. For example, 2.01 is not allowed; however, 0.2, 2.0.1, and 2.10 are allowed.

[gabrielBusta]

Element 3 is over-ridden by the pipeline to append a timestamp in such a way that the generated versions are compatible with MV3 (the timestamp is part 3 and 4). Ideally it would be (1, 2) but two part versions break a library called node-semver. Two part versions are technically valid in the semver spec, but the version in package.json must be parasable by node-semver to be a valid semantic version as far as npm is concerned. This library thinks a version must have 3 parts to be a valid semantic version (and tooling relying on this library breaks when package.json has a version that this library can't parse 🫠)

[gabrielBusta]

So allowing a third part (that is over-ridden) is a escape hatch for the addons relying on such tooling.

[gabrielBusta]

Actually, taking a look at the spec again it sounds like it requires a 3 part version.

[bhearsum]

...and this should be 4, and further down we should take the first 3 parts?

[bhearsum]

This could use a bigger explainer comment on the requirements of & what will happen with the version coming from package.json.

And as you suggested on Zoom, failing rather than throwing a warning message, if we'll remove anything other than a 0 from the third part would be better. (To avoid near silent information loss.)