1402010 - Don't parse test manifests in files metadata reading mode

Status: NEW

(Firefox Build System :: General, enhancement)

Description

[Gregory Szorc [:gps]]

We should not read test manifests in files metadata moz.build reading mode.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Bug 1402010 - Normalize paths according to finder's base;

e9416a307987f accidentally regressed various `mach file-info` commands
due to the used Finder being rooted at a different directory.

In this commit, we change the path normalization code to always
normalize paths against the relative path to the root of the Finder
being used.

Review commit: https://reviewboard.mozilla.org/r/182362/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182362/

Comment 2

[Gregory Szorc [:gps]]

Attachment: Bug 1402010 - Don't use mutable default argument value;

Review commit: https://reviewboard.mozilla.org/r/182364/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182364/

Comment 3

[Gregory Szorc [:gps]]

Attachment: Bug 1402010 - Define evaluation flags on moz.build contexts;

We have different reasons for evaluating moz.build files. Today,
the evaluation portion of the moz.build context/sandbox is mostly
the same: the differences in behavior reside at the reader layer.
This is good because we want evaluation to be as similar as possible
to avoid surprises.

As a subsequent commit will show, there are areas where evaluating
the same way everywhere has problems and there are compelling reasons
to tweak behavior under special scenarios.

In this commit, we add an annotation to all moz.build context
instances denoting what special evaluation modes are in effect.

Review commit: https://reviewboard.mozilla.org/r/182366/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182366/

Comment 4

[Gregory Szorc [:gps]]

Attachment: Bug 1402010 - Support not reading test manifests in moz.build files;

Not all consumers of moz.build evaluation are interested in test
manifest data. Normally, processing it isn't a big deal so we
just do it. However, there are a few scenarios where this is
difficult.

The difficulty processing test manifests stems from the fact
that test manifest parsers aren't part of the mozbuild Python
package. The Python modules are defined elsewhere in the repo.

In the case of reading moz.build files from version control,
a full checkout may not be available or the calling process may
not be running from the specific revision being evaluated. In
either case, these manifest processing modules may not be
available and moz.build evaluation will fail.

This commit introduces a flag on BuildReader.files_info() to
control whether test manifests are loaded. If disabled, an
evaluation flag is added and when the respective moz.build
variables are evaluated, they see the flag and short-circuit.

The module imports in testing.py were also refactored as part
of this change to swallow any import failure. We had previously
delay imported these modules as a way to work around the import
failure in certain context. If the import fails, an exception
will be thrown attempting to operate on None. This is slightly
more annoying than an explicit ImportError. The reason we can't
keep the import in the function is because "import" is processed
at block scoping, so if an import is in a block, it gets
processed, even if that line is never evaluated.

Various callers not needing to access test manifest data have
been changed to not load it.

A side effect of this change is that various `mach file-info`
commands became faster! For example,
`mach file-info bugzilla-component 'testing/**'` dropped ~500ms
from ~9000ms.

Review commit: https://reviewboard.mozilla.org/r/182368/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182368/

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8910890 [details]
Bug 1402010 - Support not reading test manifests in moz.build files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182368/diff/1-2/

Comment 6

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8910887 [details]
Bug 1402010 - Normalize paths according to finder's base;

https://reviewboard.mozilla.org/r/182362/#review187674

I don't have a ton of context on bug 1397406, but this looks ok.

::: python/mozbuild/mozbuild/frontend/mach_commands.py:173
(Diff revision 1)
>          for p in paths:
>              a = mozpath.abspath(p)
>              if not mozpath.basedir(a, [self.topsrcdir]):
>                  raise InvalidPathException('path is outside topsrcdir: %s' % p)
>  
> -            relpaths.append(mozpath.relpath(a, self.topsrcdir))
> +            query_paths.append(mozpath.relpath(a, reader.finder.base))

Should the `basedir` check above also refer to `reader.finder.base` instead of `self.topsrcdir` now?

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8910888 [details]
Bug 1402010 - Don't use mutable default argument value;

https://reviewboard.mozilla.org/r/182364/#review187676

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8910887 [details]
Bug 1402010 - Normalize paths according to finder's base;

https://reviewboard.mozilla.org/r/182362/#review187674

> Should the `basedir` check above also refer to `reader.finder.base` instead of `self.topsrcdir` now?

No. The paths being validated must reside under topsrcdir, so topsrcdir is correct.

It is only the paths that are fed into the reader/finder that need to be relative to the finder's base.

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8910890 [details]
Bug 1402010 - Support not reading test manifests in moz.build files;

https://reviewboard.mozilla.org/r/182368/#review187684

Comment 10

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8910889 [details]
Bug 1402010 - Define evaluation flags on moz.build contexts;

https://reviewboard.mozilla.org/r/182366/#review187682

If we just set this as an attribute on the config (we could even clear out `ENABLE_TESTS` in the EmptyConfig we use for our files-info reader where applicable) I think we could avoid passing these flags around everywhere.

::: python/mozbuild/mozbuild/frontend/reader.py:1355
(Diff revision 1)
>          3. Iterate over Files sub-contexts.
>          4. If the file pattern matches the file we're seeking info on,
>             apply attribute updates.
>          5. Return the most recent value of attributes.
>          """
> -        paths, _ = self.read_relevant_mozbuilds(paths)
> +        eval_flags = {'files-info'}

I don't see this flag being read/checked here or elsewhere in the series.

Comment 11

[Tom Prince [:tomprince]]

Comment on attachment 8910890 [details]
Bug 1402010 - Support not reading test manifests in moz.build files;

https://reviewboard.mozilla.org/r/182368/#review215142

::: python/mozbuild/mozbuild/testing.py:531
(Diff revision 2)
>                                         rootdir=context.config.topsrcdir,
>                                         finder=context._finder,
>                                         handle_defaults=False)
>  
>  def read_reftest_manifest(context, manifest_path):
> -    import reftest
> +    if 'no-test-manifests' in context.eval_flags:

`s/po/no/`

Comment 12

[Tom Prince [:tomprince]]

Comment on attachment 8910890 [details]
Bug 1402010 - Support not reading test manifests in moz.build files;

https://reviewboard.mozilla.org/r/182368/#review215142

> `s/po/no/`

I'm going to blame dust on my screen.

Comment 13

[Gregory Szorc [:gps]]

Attachment: Bug 1402010 - Avoid mutable default argument value

Mutable default argument values lead to surprises since Python
reuses objects on subsequent calls if the argument isn't passed!

Review commit: https://reviewboard.mozilla.org/r/214236/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214236/

Comment 14

[Gregory Szorc [:gps]]

Attachment: Bug 1402010 - Only evaluate Files() context managers in files reading mode

THIS COMMIT CURRENTLY FAILS TESTS AND SHOULD NOT BE CHECKED IN.

Reading Files() metadata is super useful. However, in order to read
moz.build files, you need to evaluate Python code. And the context
of that evaluation needs access to known primitives in the moz.build
sandbox. So, if you have old moz.build reading code and you want to
read an arbitrary moz.build file, you may run into issues where that
moz.build file uses a sandbox symbol or primitive unknown to the old
moz.build reading code. This will result in a run-time failure.

Some may say "but just use the moz.build reading code in the
repository!" This works - but only if you trust the moz.build reading
code in the repository. In some contexts - say on headless servers -
we want to extract relevant moz.build data for untrusted content. In
this case, the best we can do is use trusted code for reading moz.build
files, sandbox the execution, and hope for the best. The big problem
with that is that any commit may introduce a new moz.build sandbox
primitive and the trusted code may not be able to read a new moz.build
file using that newer primitive!

That leads us to this commit.

This commit changes the way moz.build files are evaluated in
file-traversal mode (which cares about reading the content of Files()
context managers). When the moz.build file is loaded, instead of
evaluating the code as-is, we parse it to an AST, strip out root
nodes that aren't `with Files()`, and execute that. Essentially, we
strip the moz.build file down to only the bits we care about. This
reduces the surface area of sandbox changes that can cause Files()
reading incompatibility down to changes to Files() itself. And if we
wanted to go a step further, we could strip AST nodes under Files()
that belong to variables that are unknown to the reader. This is
left as a follow-up feature.

The way this is implemented at the sandbox layer (which is generic)
is by specifying a filter of context manager names that evaluation
will be reduced to. From a higher level, we pass ['Files'].

A lot of code that hacked up the evaluation context for purposes of
Files() evaluation has been removed because none of that code runs
any more.

Open Issues:

* Tests fail
* If `with Files()` isn't a top-level AST node, we strip it.
  We probably want a lint check that ensures this pattern is never
  used.
* `with Files(), Files()` may not work properly. It definitely needs
  tests.
* If a `with Files()` references a symbol defined outside the context
  manager, evaluation fails due to the symbol being stripped. We
  could use static analysis and avoid this pattern (at the cost of
  convenience in moz.build files).
* Should we allow `include()`? If we do, we need to ensure that file's
  content is filtered as well.
* Consider setting the "only context managers" mode on the MozbuildSandbox
  instance so all calls to exec_file() contain the filter. Otherwise,
  some inner caller (like include()) may call exec_file() without the
  AST stripping and our feature will be defeated.

I'm submitting this for consideration to see if there is interest
in this approach. If not, then the only viable alternative I see
for the futures of Files() metadata if we want trusted execution
is to extract Files() to separate files that can be statically
evaluated without executing Python. This *is* probably the best
path forward. But it fragments data between moz.build and
*something else*.

Review commit: https://reviewboard.mozilla.org/r/214238/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214238/

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 8943880 [details]
Bug 1402010 - Only evaluate Files() context managers in files reading mode

This can't land yet. I'm just looking for your feedback on whether you like the approach.

Essentially, Files() metadata is nice. But it is incompatible with out-of-tree readers that need to securely extract file metadata from untrusted commits because it is impossible to keep the moz.build reading code up-to-date. We need to do *something* to support the moz.build web service (bug 1354791) and other consumers. This is the best strategy I can think of that doesn't involve "extract the metadata definition to something not moz.build."

The reason this came up today is because Connor is working on some things to define code commit policies in-repo. That can either piggyback on top of Files(), we can adapt Files() metadata so it can work more reliably, or we invent a one-off for Connor's use case. I was hoping to not create a one-off. So I think our choices are "make moz.build reader more robust" (this PoC) or "move files metadata out of moz.build."

Comment 16

[Gregory Szorc [:gps]]

Connor: herein I attempt to make moz.build evaluation safe and less error prone. Read the patch if you want to see some face melting advanced Python :)

Comment 17

[Phabricator Automation]

Attachment: Bug 1402010 - Only evaluate Files() context managers in files reading mode

Comment 18

[(not currently active) Ted Mielczarek]

Comment on attachment 8943880 [details]
Bug 1402010 - Only evaluate Files() context managers in files reading mode

https://reviewboard.mozilla.org/r/214238/#review228692

I don't have any real concerns with this approach. We should probably think longer-term about whether keeping this data in moz.build files is the right thing to do.

::: commit-message-e4eb0:56
(Diff revision 1)
> +* `with Files(), Files()` may not work properly. It definitely needs
> +  tests.
> +* If a `with Files()` references a symbol defined outside the context
> +  manager, evaluation fails due to the symbol being stripped. We
> +  could use static analysis and avoid this pattern (at the cost of
> +  convenience in moz.build files).

I think at a bare minimum we ought to have some sort of lint or test for these few things you've called out before landing this. I don't think any of these are unreasonable--Files metadata shouldn't be incredibly complicated, and I'd be surprised if we actually had a good reason to violate any of these.

::: commit-message-e4eb0:70
(Diff revision 1)
> +in this approach. If not, then the only viable alternative I see
> +for the futures of Files() metadata if we want trusted execution
> +is to extract Files() to separate files that can be statically
> +evaluated without executing Python. This *is* probably the best
> +path forward. But it fragments data between moz.build and
> +*something else*.

Given the fact that this data is already in moz.build files I think these changes are useful.

However, given glob's moz.vendor proposal, I wonder if it wouldn't make sense longer-term to just move the Files metadata into that same format? (I certainly wish we had some half-measure available where we could have the nice Pythonic syntax of moz.build files but without Turing completeness!)

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.