Code analysis found 2 defects in diff 773526:

• 1 defect found by py-black (Mozlint)
• 1 defect found by py-ruff (Mozlint)

You can run this analysis locally with:

• ./mach lint --warnings --outgoing

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 773526.

2 defects closed compared to the previous diff 773526.

If you see a problem in this automated review, please report it here.

@sergesanspaille can you provide a try run with 1-2 raptor-browsertime linux pageload tests?

This patch breaks ./mach browsertime locally:

sparky@sparky-ThinkPad-X1-Carbon-Gen-9:~/mozilla-source/mozilla-central$ ./mach browsertime --setup --clobber
 0:00.24 [INFO] This command should be used for browsertime setup only.
If you are looking to run performance tests on your patch, use `./mach raptor --browsertime` instead.

You can get visual-metrics by using the --browsertime-video and --browsertime-visualmetrics. Here is a sample command for raptor-browsertime: 
	`./mach raptor --browsertime -t amazon --browsertime-video --browsertime-visualmetrics`

See this wiki page for more information if needed: https://wiki.mozilla.org/TestEngineering/Performance/Raptor/Browsertime


 0:05.26 Using artifact from local cache: /home/sparky/.mozbuild/cache/browsertime/2e82796d80fdc5f6-ffmpeg-4.4.1-i686-static.tar.xz
 0:05.26 Unpacking temporary location /home/sparky/.mozbuild/cache/browsertime/2e82796d80fdc5f6-ffmpeg-4.4.1-i686-static.tar.xz
Error running mach:

    mach browsertime --setup --clobber

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.
You can invoke ``./mach busted`` to check if this issue is already on file. If it
isn't, please use ``./mach busted file browsertime`` to report it. If ``./mach busted`` is
misbehaving, you can also inspect the dependencies of bug 1543241.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

AttributeError: 'Version' object has no attribute 'version'

  File "/home/sparky/mozilla-source/mozilla-central/tools/browsertime/mach_commands.py", line 670, in browsertime
    return setup_browsertime(
  File "/home/sparky/mozilla-source/mozilla-central/tools/browsertime/mach_commands.py", line 317, in setup_browsertime
    if not setup_helper.check_node_executables_valid():
  File "/home/sparky/mozilla-source/mozilla-central/tools/lint/eslint/setup_helper.py", line 405, in check_node_executables_valid
    node_path, version = find_node_executable()
  File "/home/sparky/mozilla-source/mozilla-central/python/mozbuild/mozbuild/nodeutil.py", line 90, in find_node_executable
    return nodejs_exe, version.version

Change done. @sparky could you check it doesn't disturb your workflow?

./mach browsertime --setup --clobber

works on my local setup.

I've also triggered some mozperftest unit tests since they weren't triggered by the bot, but I know we use the nodeutil file there: https://treeherder.mozilla.org/jobs?repo=try&revision=26ded73d75d4cebe35c9a96fcbd234aece6ac2d9&searchStr=perftest

In D190540#6329890, @saschanaz wrote:

Okay, a couple of things:

1. _cmp() is still used by LooseVersion and should be kept. 👍
2. This patch still changes the behavior:

> ./mach python
> from mozrelease.versions import MozillaVersion
> MozillaVersion("3.6.3plugin1") // This gives ModernMozillaVersion on m-c but LooseVersion now. Not covered by the test
> MozillaVersion("1.0.0.1") // This gives AncientMozillaVersion on m-c but Version now, because packaging.version.Version supports the x.x.x.x format while distutils does not. Not covered by the test

I wonder we still need ModernMozillaVersion and AncientMozillaVersion at all? The only real caller, update-verify-config-creator, downloads a list and parse it through MozillaVersion, so I tried it and got a URL: https://product-details.mozilla.org/1.0/firefox.json, but it did not include any special version string like 3.6.3plugin1.

I managed to run the script with ./mach python testing/mozharness/scripts/release/update-verify-config-creator.py --product firefox --platform win32 --archive-prefix https://archive.mozilla.org/pub --to-version 59.0b5 --last-watershed 57.0b10 --include-version ".+" --branch-prefix mozilla --app-name browser and it never failed to parse the versions, so I guess it's now fine to throw the special MozillaVersions away?

But the removal can be done in a separate patch, 👍 for now.

Thanks for the detailed answer. I Think I understand why we have this: version is not honoring version_re... I think I can improve this, let me have a look.

@saschanaz : I ended up providing a limited implementation of StrictVersion that matches the behavior of distutils' because I was uncomfortable with the loss of validation against regular expression. Associated try build: https://treeherder.mozilla.org/jobs?repo=try&revision=b1d015f86285cf069bbc65bd269529758ff7f41c (I've fixed the formatting issue in the above since then, don't want to restart a build just for that)

In D190540#6331385, @sergesanspaille wrote:

@saschanaz : I ended up providing a limited implementation of StrictVersion that matches the behavior of distutils' because I was uncomfortable with the loss of validation against regular expression. Associated try build: https://treeherder.mozilla.org/jobs?repo=try&revision=b1d015f86285cf069bbc65bd269529758ff7f41c (I've fixed the formatting issue in the above since then, don't want to restart a build just for that)

The point of the custom version classes were to loosen the rules and cover more version strings, but those strings are now either nonexistent or covered by Version, so I don't really see the point...

Code analysis found 1 defect in diff 776575:

• 1 defect found by py-black (Mozlint)

3 defects closed compared to the previous diff 776574.

You can run this analysis locally with:

• ./mach lint --warnings --outgoing

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 776575.

1 defect closed compared to the previous diff 776575.

If you see a problem in this automated review, please report it here.