-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update the pipeline to generate addon build versions that are compatible with MV3 #215
Changes from 3 commits
b0c9f2b
ac21012
cdcc489
85c602c
0ff274b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -75,17 +75,32 @@ def write_package_info(package_info): | |
|
||
def get_buildid(): | ||
now = datetime.utcnow() | ||
return now.strftime("%Y%m%d.%H%M%S") | ||
# note the `-` (hyphen) in `%-H` | ||
# this removes the leading zero | ||
# from the hour (leading zeros are not allowed) | ||
return now.strftime("%Y%m%d.%-H%M%S") | ||
|
||
|
||
def get_buildid_version(version): | ||
"""Version schema check+append a `buildid{buildid}` to ensure unique version.""" | ||
# XXX is there a more precise schema we should verify ourselves against? | ||
if len(version.split(".")) not in (1, 2, 3): | ||
raise Exception("{version} has too many version parts!") | ||
if "buildid" in version: | ||
raise Exception("{version} already has a buildid specified!") | ||
buildid_version = f"{version}buildid{get_buildid()}" | ||
"""Check the version's formating and append `date.time` as a buildid to ensure a unique version. | ||
The addon's in-tree `package.json` file specifies `major.minor.0`. | ||
The format of the resulting version field is: | ||
<number>.<number>.%Y%m%d.%-H%M%S | ||
""" | ||
parts = version.split(".") | ||
if len(parts) not in (1, 2, 3): | ||
raise Exception( | ||
f"{version} has an invalid number of version parts! The pipeline supports a `major.minor.0` version format in the extension's manifest" | ||
) | ||
# Append (or override) the last two parts of the version with a timestamp to ensure a unique version that is compliant with MV3. | ||
if len(parts) == 3: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ...and this should be 4, and further down we should take the first 3 parts? |
||
# Print a noisy warning if we override an element in the extension's version field. | ||
if parts[2] != "0": | ||
msg = f"! WARNING: THE 3RD ELEMENT IN THE VERSION {version} IS {parts[2]} NOT 0. THIS VALUE WILL BE OVERRIDEN BY THE PIPELINE !" | ||
print(f"{'!' * len(msg)}\n\n{msg}\n\n{'!' * len(msg)}") | ||
parts = parts[:2] | ||
version = ".".join(parts) | ||
buildid_version = f"{version}.{get_buildid()}" | ||
print(f"Buildid version is {buildid_version}") | ||
return buildid_version | ||
|
||
|
@@ -128,11 +143,35 @@ def mkdir(path): | |
def get_hash(path, hash_alg="sha256"): | ||
h = hashlib.new(hash_alg) | ||
with open(path, "rb") as fh: | ||
for chunk in iter(functools.partial(fh.read, 4096), b''): | ||
for chunk in iter(functools.partial(fh.read, 4096), b""): | ||
h.update(chunk) | ||
return h.hexdigest() | ||
|
||
|
||
def is_version_mv3_compliant(version): | ||
# Split the version string by dots | ||
parts = version.split(".") | ||
|
||
# Check if there are 1 to 4 parts | ||
if len(parts) < 1 or len(parts) > 4: | ||
return False | ||
|
||
for part in parts: | ||
# Check if the part is a number | ||
if not part.isdigit(): | ||
return False | ||
# Check that the part doesn't have leading zeros (unless it's "0") | ||
if part[0] == "0" and part != "0": | ||
return False | ||
# Convert part to integer to check the digit count | ||
num = int(part) | ||
# Check if the integer has more than 9 digits | ||
if num > 999999999: | ||
return False | ||
|
||
return True | ||
|
||
|
||
def check_manifest(path, buildid_version): | ||
xpi = ZipFile(path, "r") | ||
manifest = {} | ||
|
@@ -150,20 +189,35 @@ def check_manifest(path, buildid_version): | |
continue | ||
_found = True | ||
if not _id.endswith(ID_ALLOWLIST): | ||
raise Exception(f"{_key}.gecko.id {_id} must end with one of the following suffixes!\n{ID_ALLOWLIST}") | ||
raise Exception( | ||
f"{_key}.gecko.id {_id} must end with one of the following suffixes!\n{ID_ALLOWLIST}" | ||
) | ||
else: | ||
print(f"Add-on id {_id} matches the allowlist.") | ||
if manifest["version"] != buildid_version: | ||
raise Exception(f"{manifest['version']} doesn't match buildid version {buildid_version}!") | ||
raise Exception( | ||
f"{manifest['version']} doesn't match buildid version {buildid_version}!" | ||
) | ||
if manifest["manifest_version"] == 3 and not is_version_mv3_compliant(manifest["version"]): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seeing the check against the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
raise Exception( | ||
( | ||
f"The version in {manifest_name} is {manifest['version']}, which is not MV3 compliant. " | ||
"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)." | ||
) | ||
) | ||
if not _found: | ||
raise Exception("Can't find addon ID in manifest.json!") | ||
|
||
|
||
def main(): | ||
test_var_set([ | ||
"ARTIFACT_PREFIX", | ||
"XPI_NAME", | ||
]) | ||
test_var_set( | ||
[ | ||
"ARTIFACT_PREFIX", | ||
"XPI_NAME", | ||
] | ||
) | ||
|
||
artifact_prefix = os.environ["ARTIFACT_PREFIX"] | ||
xpi_name = os.environ["XPI_NAME"] | ||
|
@@ -204,10 +258,10 @@ def main(): | |
run_command(["npm", "install"]) | ||
run_command(["npm", "run", "build"]) | ||
|
||
if 'XPI_ARTIFACTS' in os.environ: | ||
if "XPI_ARTIFACTS" in os.environ: | ||
xpi_artifacts = os.environ["XPI_ARTIFACTS"].split(";") | ||
else: | ||
xpi_artifacts = glob.glob('*.xpi') + glob.glob("**/*.xpi") | ||
xpi_artifacts = glob.glob("*.xpi") + glob.glob("**/*.xpi") | ||
|
||
all_paths = [] | ||
for artifact in xpi_artifacts: | ||
|
@@ -232,4 +286,4 @@ def main(): | |
fh.write(json.dumps(build_manifest, indent=2, sort_keys=True)) | ||
|
||
|
||
__name__ == '__main__' and main() | ||
__name__ == "__main__" and main() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be (1,2,3,4)? From the docs:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 inpackage.json
must be parasable bynode-semver
to be a valid semantic version as far asnpm
is concerned. This library thinks a version must have 3 parts to be a valid semantic version (and tooling relying on this library breaks whenpackage.json
has a version that this library can't parse 🫠)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So allowing a third part (that is over-ridden) is a escape hatch for the addons relying on such tooling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, taking a look at the spec again it sounds like it requires a 3 part version.