1313306 - Relax the rules for --help dependencies

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

Before my cleaned-up patch queue grows too deep, let's get what is ready to land landed. The patch series I'm going to push ends up removing many dummy invisible --help dependencies: down from 215 to 33.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Bug 1313306 - Add a placeholder python configure linter.

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

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1313306 - Move --help dependency checks to the linter.

Missing such dependencies shouldn't impair running configure itself
after local modifications, but they are currently required for
(mostly) documentation purpose. Which means they are better done in
the linter.

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

Comment 3

[Mike Hommey [:glandium]]

Attachment: Bug 1313306 - Don't expose os.path.{exists,isdir,isfile} to python configure without an @imports.

We want functions without an @imports to not have any side effects, and
to not use external resources. So remove the few functions we expose from
os.path without @imports('os') that do.

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

Comment 4

[Mike Hommey [:glandium]]

Attachment: Bug 1313306 - Relax the rules for --help dependencies.

--help dependencies currently help identify functions that will run when
running configure --help, which we don't want to have spreading too
much. OTOH, when such functions have no side effect, it's not really
that important to have them explicitly marked.

So, allow missing --help dependencies for functions that:
- don't use @imports
- don't have a closure
- don't use global variables

This is a first step towards entirely removing the --help markings (the
end goal being that --help dependencies will indicate actual --help
dependencies). As such, we don't really care about updating the lint
error message.

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

Comment 5

[Mike Hommey [:glandium]]

Attachment: Bug 1313306 - Allow @depends(when=something) without additional dependencies.

Also allow when=True/False to avoid the chicken-egg problem of using
a generic `when` to use in replacement of @depends('--help') for things
like @dependable.

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

Comment 6

[Mike Hommey [:glandium]]

Attachment: Bug 1313306 - Remove --help dependencies from @dependable and delayed_getattr.

This sadly requires to replace a few @dependendable to pass the lint
check.

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

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8805024 [details]
Bug 1313306 - Add a placeholder python configure linter.

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

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8805025 [details]
Bug 1313306 - Move --help dependency checks to the linter.

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

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8805026 [details]
Bug 1313306 - Don't expose os.path.{exists,isdir,isfile} to python configure without an @imports.

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

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8805027 [details]
Bug 1313306 - Relax the rules for --help dependencies.

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

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8805028 [details]
Bug 1313306 - Allow @depends(when=something) without additional dependencies.

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

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8805029 [details]
Bug 1313306 - Remove --help dependencies from @dependable and delayed_getattr.

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

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8805024 [details]
Bug 1313306 - Add a placeholder python configure linter.

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

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 8805025 [details]
Bug 1313306 - Move --help dependency checks to the linter.

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

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 8805026 [details]
Bug 1313306 - Don't expose os.path.{exists,isdir,isfile} to python configure without an @imports.

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

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8805027 [details]
Bug 1313306 - Relax the rules for --help dependencies.

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

Comment 17

[Mike Hommey [:glandium]]

Comment on attachment 8805028 [details]
Bug 1313306 - Allow @depends(when=something) without additional dependencies.

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

Comment 18

[Mike Hommey [:glandium]]

Comment on attachment 8805029 [details]
Bug 1313306 - Remove --help dependencies from @dependable and delayed_getattr.

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

Comment 19

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8805024 [details]
Bug 1313306 - Add a placeholder python configure linter.

https://reviewboard.mozilla.org/r/88846/#review88610

::: python/mozbuild/mozbuild/configure/lint.py:7
(Diff revision 3)
> +import inspect
> +from StringIO import StringIO
> +from collections import defaultdict
> +from contextlib import contextmanager

Some of these imports seem to belong in a later commit.

::: python/mozbuild/mozbuild/test/configure/lint.py:9
(Diff revision 3)
> +
> +from __future__ import absolute_import, print_function, unicode_literals
> +
> +import os
> +import unittest
> +from StringIO import StringIO

This import isn't used in this commit.

Comment 20

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8805024 [details]
Bug 1313306 - Add a placeholder python configure linter.

https://reviewboard.mozilla.org/r/88846/#review88614

Comment 21

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8805025 [details]
Bug 1313306 - Move --help dependency checks to the linter.

https://reviewboard.mozilla.org/r/88848/#review88620

Comment 22

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8805026 [details]
Bug 1313306 - Don't expose os.path.{exists,isdir,isfile} to python configure without an @imports.

https://reviewboard.mozilla.org/r/88850/#review88622

::: python/mozbuild/mozbuild/test/configure/common.py:91
(Diff revision 3)
>  
>          vfs = ConfigureTestVFS(paths)
>  
> -        self.OS = ReadOnlyNamespace(path=ReadOnlyNamespace(**{
> -            k: v if k not in ('exists', 'isfile')
> -            else getattr(vfs, k)
> +        os_path = {
> +            k: getattr(vfs, k)
> +            for k in dir(vfs)

This could probably fit on the line above.

Comment 23

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8805027 [details]
Bug 1313306 - Relax the rules for --help dependencies.

https://reviewboard.mozilla.org/r/88852/#review88634

Comment 24

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8805028 [details]
Bug 1313306 - Allow @depends(when=something) without additional dependencies.

https://reviewboard.mozilla.org/r/88854/#review88636

::: python/mozbuild/mozbuild/configure/__init__.py:288
(Diff revision 3)
> +        self._always = DependsFunction(self, lambda: True, [])
> +        self._never = DependsFunction(self, lambda: False, [])
> +

The prior commit seems to mentions these attributes... does this commit actually go first?

Comment 25

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8805029 [details]
Bug 1313306 - Remove --help dependencies from @dependable and delayed_getattr.

https://reviewboard.mozilla.org/r/88856/#review88638

Comment 26

[Mike Hommey [:glandium]]

Comment on attachment 8805024 [details]
Bug 1313306 - Add a placeholder python configure linter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88846/diff/3-4/

Comment 27

[Mike Hommey [:glandium]]

Comment on attachment 8805025 [details]
Bug 1313306 - Move --help dependency checks to the linter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88848/diff/3-4/

Comment 28

[Mike Hommey [:glandium]]

Comment on attachment 8805026 [details]
Bug 1313306 - Don't expose os.path.{exists,isdir,isfile} to python configure without an @imports.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88850/diff/3-4/

Comment 29

[Mike Hommey [:glandium]]

Comment on attachment 8805027 [details]
Bug 1313306 - Relax the rules for --help dependencies.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88852/diff/3-4/

Comment 30

[Mike Hommey [:glandium]]

Comment on attachment 8805028 [details]
Bug 1313306 - Allow @depends(when=something) without additional dependencies.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88854/diff/3-4/

Comment 31

[Mike Hommey [:glandium]]

Comment on attachment 8805029 [details]
Bug 1313306 - Remove --help dependencies from @dependable and delayed_getattr.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88856/diff/3-4/

Comment 32

[Mike Hommey [:glandium]]

Comment on attachment 8805029 [details]
Bug 1313306 - Remove --help dependencies from @dependable and delayed_getattr.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88856/diff/4-5/

Comment 33

[Mike Hommey [:glandium]]

Comment on attachment 8805026 [details]
Bug 1313306 - Don't expose os.path.{exists,isdir,isfile} to python configure without an @imports.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88850/diff/4-5/

Comment 34

[Mike Hommey [:glandium]]

Comment on attachment 8805027 [details]
Bug 1313306 - Relax the rules for --help dependencies.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88852/diff/4-5/

Comment 35

[Mike Hommey [:glandium]]

Comment on attachment 8805028 [details]
Bug 1313306 - Allow @depends(when=something) without additional dependencies.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88854/diff/4-5/

Comment 36

[Mike Hommey [:glandium]]

Comment on attachment 8805029 [details]
Bug 1313306 - Remove --help dependencies from @dependable and delayed_getattr.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88856/diff/5-6/

Comment 37

[Mike Hommey [:glandium]]

Comment on attachment 8805026 [details]
Bug 1313306 - Don't expose os.path.{exists,isdir,isfile} to python configure without an @imports.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88850/diff/5-6/

Comment 38

[Mike Hommey [:glandium]]

Comment on attachment 8805027 [details]
Bug 1313306 - Relax the rules for --help dependencies.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88852/diff/5-6/

Comment 39

[Mike Hommey [:glandium]]

Comment on attachment 8805028 [details]
Bug 1313306 - Allow @depends(when=something) without additional dependencies.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88854/diff/5-6/

Comment 40

[Mike Hommey [:glandium]]

Comment on attachment 8805029 [details]
Bug 1313306 - Remove --help dependencies from @dependable and delayed_getattr.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88856/diff/6-7/

Comment 41

[Mike Hommey [:glandium]]

Comment on attachment 8805026 [details]
Bug 1313306 - Don't expose os.path.{exists,isdir,isfile} to python configure without an @imports.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88850/diff/6-7/

Comment 42

[Mike Hommey [:glandium]]

Comment on attachment 8805027 [details]
Bug 1313306 - Relax the rules for --help dependencies.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88852/diff/6-7/

Comment 43

[Mike Hommey [:glandium]]

Comment on attachment 8805028 [details]
Bug 1313306 - Allow @depends(when=something) without additional dependencies.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88854/diff/6-7/

Comment 44

[Mike Hommey [:glandium]]

Comment on attachment 8805029 [details]
Bug 1313306 - Remove --help dependencies from @dependable and delayed_getattr.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88856/diff/7-8/

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/caf5e078e7ef
Add a placeholder python configure linter. r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/0e64d3d27520
Move --help dependency checks to the linter. r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/b6be0e9e3e1e
Don't expose os.path.{exists,isdir,isfile} to python configure without an @imports. r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/a801ef597f1f
Relax the rules for --help dependencies. r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/0ac9e88ff47b
Allow @depends(when=something) without additional dependencies. r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/8158226d06a0
Remove --help dependencies from @dependable and delayed_getattr. r=chmanchester

Comment 46

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/caf5e078e7ef
https://hg.mozilla.org/mozilla-central/rev/0e64d3d27520
https://hg.mozilla.org/mozilla-central/rev/b6be0e9e3e1e
https://hg.mozilla.org/mozilla-central/rev/a801ef597f1f
https://hg.mozilla.org/mozilla-central/rev/0ac9e88ff47b
https://hg.mozilla.org/mozilla-central/rev/8158226d06a0

Comment 47

[Bill Gianopoulos [:WG9s]]

Seems odd that a relaxing rules bug results in build failures.