1598350 - Run glean_parser for linting metrics.yaml in mozilla-central

Status: RESOLVED DUPLICATE of bug 1602773

(Data Platform and Tools :: Glean: SDK, enhancement, P1)

Description

[Alessio Placitelli [:Dexter]]

We currently have metrics.yaml in m-c which is used by the Glean SDK when GeckoView is used in Glean-aware products.

We should make sure this file is linted using the glean_parser and that we break build if it isn't, otherwise we will end up breaking Android Components and figuring that out when is too late.

Comment 1

[Alessio Placitelli [:Dexter]]

Hi :glob!

I'm not sure if you're the right person for this answer, would you kindly redirect if you're not?

We need to call glean_parser (Python 3.7, to be vendored or fetched when setting up the environment) at build time, in order to lint this file and fail build (or a test?) if errors are detected.

Is there any way to invoke python3 scripts from the build system?

Comment 2

[Chris Manchester (limited bugmail, email directly)]

I believe we have the ability to run tests with Python 3, that's probably the thing to do here. :ahal may have more specific advice.

Comment 3

[Andrew Halberstadt [:ahal]]

The Python 3.7 requirement will definitely complicate matters. I believe both the build and python-tests run with 3.5 in CI. If this is something you want to integrate as part of the build itself you'll need to reduce the glean_parser requirement down to 3.5 full stop (I'm not a build peer, this is just my understanding). If it's something external to the build (i.e run as part of make check or as a separate task), then you can likely continue using 3.7 but may run into issues where workers don't have that installed.

I agree a python-test seems the most ideal (we do run some of them in make check atm, though I don't believe we run any under Python 3 yet). Other possibilities are a linter, or a standalone task in taskcluster/ci/source-test.

Comment 4

[Alessio Placitelli [:Dexter]]

(In reply to Andrew Halberstadt [:ahal] from comment #3)

The Python 3.7 requirement will definitely complicate matters. I believe both the build and python-tests run with 3.5 in CI. If this is something you want to integrate as part of the build itself you'll need to reduce the glean_parser requirement down to 3.5 full stop (I'm not a build peer, this is just my understanding).

@mshal, flagging you as Chris is away. Do you know what's the Python3 version our build system is using? On Windows, MozillaBuild seems to be shipping with Python 3.7.4, but I could not find any specific version reference in our code, other than "Something >= 3.5".

Comment 5

[Michael Shal [:mshal]]

(In reply to Alessio Placitelli [:Dexter] from comment #4)

(In reply to Andrew Halberstadt [:ahal] from comment #3)

The Python 3.7 requirement will definitely complicate matters. I believe both the build and python-tests run with 3.5 in CI. If this is something you want to integrate as part of the build itself you'll need to reduce the glean_parser requirement down to 3.5 full stop (I'm not a build peer, this is just my understanding).

@mshal, flagging you as Chris is away. Do you know what's the Python3 version our build system is using? On Windows, MozillaBuild seems to be shipping with Python 3.7.4, but I could not find any specific version reference in our code, other than "Something >= 3.5".

Right, 3.5 is the minimum python3 version currently required. On our build machines, it looks like we have these exact versions installed:

Linux: 3.5.3
Mac: 3.5.3
Windows: 3.6.5

Note that the scripts invoked during the build currently use python2.7, though work is underway to start using py3 instead. See eg: bug 1599658

Comment 6

[Alessio Placitelli [:Dexter]]

(In reply to Michael Shal [:mshal] from comment #5)

Right, 3.5 is the minimum python3 version currently required. On our build machines, it looks like we have these exact versions installed:

Thanks. Looks like our main blocker is depending on Python 3.7, then :)

@Mike, is there any way to use Python 3.5 instead of Python 3.7 in glean_parser? How much do we rely on 3.7 features?

Comment 7

[Michael Droettboom [:mdroettboom]]

I haven't done an exhaustive search, but the main Python 3.7-only feature we use is the dataclasses library. It looks like there is a backport for that to Python 3.6 here, but not all the way back to 3.5.

Comment 8

[Michael Droettboom [:mdroettboom]]

Also see this relevant issue about supporting dataclasses on Python 3.5. We would probably be better off rewriting glean_parser to not use dataclasses, which could definitely be done without breaking external API, there would just be a lot more boilerplate.

Comment 9

[Alessio Placitelli [:Dexter]]

Hi Chris,

how likely would it be to bump the minimum Python build version of Firefox to 3.6/3.7 in order to support this? Would it be a reasonable request? I'm fairly sure this has many dependencies that I'm not aware of, but I wanted to try asking anyway :)

Comment 10

[Alessio Placitelli [:Dexter]]

FWIW, looks like Python 3.5 will be EOL by September 2020

Comment 11

[Chris Manchester (limited bugmail, email directly)]

I don't know if we'd go as far as requiring a different version to build for this, but there's no prohibition against using more recent versions in tests that run in CI. Would it be sufficient to run this test per-checkin on one platform (Linux for instance)? It may be feasible to set up a docker image with the right python version for a specific test.

Comment 12

[Alessio Placitelli [:Dexter]]

(In reply to Chris Manchester (:chmanchester) from comment #11)

I don't know if we'd go as far as requiring a different version to build for this, but there's no prohibition against using more recent versions in tests that run in CI.

Thank you for following up. We will need to add this as part of the build system in the future, as we want feature parity with our current Firefox Telemetry: it breaks the build if something weird is detected in our registry files.

We'll go on and port to Python 3.5.3.

Comment 13

[Alessio Placitelli [:Dexter]]

Untaking: this is currently blocked by dependent bugs.

Comment 14

[Alessio Placitelli [:Dexter]]

Works for this is happening in the bug being duplicated against.