Add several undocumented flags (--bazel, --package-root and --cache-map) in support of Bazel #4759

gvanrossum · 2018-03-19T19:36:27Z

The --bazel flag make cache files hermetic by using relative paths and setting mtime to zero; effectively the presence of a cache file prevents reading of the source file (though the source file must still exist).

The --cache-map flag specifies a mapping from source files to cache files that overrides the usual way of finding the locations for cache files; e.g.

mypy --cache-map foo.py foo.meta.json foo.data.json bar.py bar.meta.json bar.data.json -- foo.py bar.py

There must be any number of triples (source, meta, data) with the additional constraints that the source must end in .py or .pyi, meta must end in .meta.json, and data must end in .data.json. For files in the cache map, the default cache directory (--cache-dir) is ignored, and if the cache files named in the --cache-map don't exist they will be written. For files not in the cache map, the --cache-dir is still used.

(NOTE: You still need to pass the source files separately, and the list of --cache-map triples must be followed by another flag or by --, as in the example above.)

- Force relative paths. - Force mtimes to zero. - Locate cache files alongside source files.

…if present

sixolet

Yes, this looks like about the tack I was thinking of lo so many months ago.

sixolet · 2018-03-22T00:49:38Z

mypy/build.py

 Graph = Dict[str, 'State']


 def getmtime(name: str) -> int:
    return int(os.path.getmtime(name))


+def relpath(path: str) -> str:
+    if not path or path[0] not in SEPARATORS:


"""Returns the path relative to the current working directory."""

sixolet · 2018-03-22T00:52:31Z

mypy/build.py

      cache_dir: cache directory
      pyversion: Python version (major, minor)

    Returns:
      A tuple with the file names to be used for the meta JSON and the
      data JSON, respectively.
    """
+    pair = manager.options.cache_map.get(path)


It feels naïvely like it'd be cleaner to always use the cache map in the presence of a cache map at all; I don't know that the hybrid mode of looking in a cache dir and a cache map file would be very meaningful, and it certainly sounds hard to debug.

That might well be the case but I haven't solved what to do about typeshed yet, so without keeping cache_dir as a fallback, typeshed will always read the source.

sixolet · 2018-03-22T00:55:03Z

mypy/main.py

@@ -381,6 +382,15 @@ def add_invertible_flag(flag: str,
    # --local-partial-types disallows partial types spanning module top level and a function
    # (implicitly defined in fine-grained incremental mode)
    parser.add_argument('--local-partial-types', action='store_true', help=argparse.SUPPRESS)
+    # --bazel changes some behaviors for use with Bazel (https://bazel.build).
+    parser.add_argument('--bazel', action='store_true', help=argparse.SUPPRESS)
+    # --cache-map FILE ... gives a mapping from source files to cache files.


I am interested in what happened to the idea of dropping cache files next to source files. I remember choosing the same option you did here, but I forget why, because now-me had the idea that cache files next to source would be cleaner for some reason.

I tried to write this up in my overly long status report in #3912 -- Dropbox's build environment puts the output files in a different directory than the source files. It seems outputs (and inputs) must be File objects, the only way to create those is ctx.actions.declare_file(), which always puts it it in the output directory. (The File objects for the sources come from ctx.files.srcs, but I don't think I can add new ones via that approach.)

That is correct -- you cannot add anything via ctx.files.srcs. You should assume that the source tree is read-only.

If you declare a file, you must create it within the action (Bazel checks this).

sixolet · 2018-03-22T00:55:39Z

mypy/main.py

+        assert n % 3 == 0, "--cache-map requires one or more triples (see source)"
+        for i in range(0, n, 3):
+            source, meta_file, data_file = special_opts.cache_map[i:i + 3]
+            assert source not in options.cache_map


In final code probably wants to be something other than an assert

gvanrossum · 2018-03-26T19:39:49Z

I have to revise this, see #3912 (comment) why. (I can't solve the implicit __init__.py problem in pure Skylark, I need to support this in mypy.)

…only)

gvanrossum · 2018-03-29T03:02:55Z

I probably have to go back here and add a new flag, --package-roots DIR DIR .... I've finally hit a large enough sample of our codebase to hit a situation where a "source" file is actually generated by another rule/action, and in that case the file exists in a different tree. Fortunately in Skylark I can figure out when this is the case: File objects have a root attribute which is another File object pointing there, and it's empty for genuine sources but points to the output tree for generated sources. So I can write some Skylark that computes the necessary --package-roots arguments. I was somewhat anticipating this with the naming of the arguments in the last diff, but I'll have to switch it from a bool to List[str].

sixolet · 2018-03-30T20:17:17Z

I'm following along. I think my version of this a few months ago didn't fake having __init__.py, it allowed dirs to be treated as packages even in the absence of __init__.py. I think I like your version better.

I also had to do something similar to your suggested --package-roots change, though at the time I thought it was just because of the weird way my particular codebase is organized.

gvanrossum · 2018-05-02T22:06:34Z

This is now a bit more battle-tested (see latest commit) and I'm ready for a full-scale review to land it.

gvanrossum · 2018-05-22T00:16:06Z

Note that "gvanrossum added some commits 17 days ago" is a lie -- I merged today, fixing two (fortunately simple) conflicts. @msullivan it seems you're "it" for the review of this diff...

msullivan

Some comments, mostly seems fine.

I don't love all the extra logic in the file system cache path, but there doesn't seem to be a much nicer way.

msullivan · 2018-05-25T06:41:52Z

mypy/build.py

@@ -1272,12 +1287,12 @@ def validate_meta(meta: Optional[CacheMeta], id: str, path: Optional[str],
    fine_grained_cache = manager.use_fine_grained_cache()

    size = st.st_size
-    if size != meta.size and not fine_grained_cache:


Agreed, such a comment would be useful

msullivan · 2018-05-25T06:47:42Z

mypy/build.py

+    else:
+        # For Bazel, make all paths relative (else Bazel caching is thwarted).
+        # And also patch tree.path, which might have been made absolute earlier.
+        tree.path = path = os.path.relpath(path)


Maybe factor a bunch of the abspath/relpath casing into a function?

Why does the bazel case need to update tree.path.

Also, could we instead just always get the relpath? Do we need absolute paths outside bazel mode?

msullivan · 2018-05-25T06:49:31Z

mypy/fscache.py

            # Take a copy to get rid of associated traceback and frame objects.
            # Just assigning to __traceback__ doesn't free them.
            self.stat_error_cache[path] = copy_os_error(err)
            raise err
        self.stat_cache[path] = st
        return st

+    def init_under_package_root(self, path: str) -> bool:


Could use some documentation. I'm not sure what's the goal/what's going on here.

msullivan · 2018-05-25T07:20:26Z

mypy/main.py

@@ -633,6 +650,57 @@ def add_invertible_flag(flag: str,
            report_dir = val
            options.report_dirs[report_type] = report_dir

+    # Validate and normalize --package-root.


Pull this logic out to functions?

msullivan · 2018-05-25T07:25:07Z

test-data/unit/check-incremental.test

@@ -4378,3 +4378,14 @@ from d import k
 [file t.py.2]
 from d import k
 # dummy change
+
+[case testBazelFlagIgnoresFileChanges]


Are there any other --bazel behaviors we can check in tests without too much pain?

msullivan · 2018-05-25T07:26:51Z

mypy/build.py

    if old_interface_hash == interface_hash:
        # If the interface is unchanged, the cached data is guaranteed
        # to be equivalent, and we only need to update the metadata.
-        data_mtime = getmtime(data_json)


msullivan · 2018-05-25T07:29:05Z

mypy/build.py

@@ -71,6 +71,12 @@
 PYTHON_EXTENSIONS = ['.pyi', '.py']


+if os.altsep:


msullivan · 2018-05-25T07:43:49Z

mypy/fscache.py

-            raise
+        dirname, basename = os.path.split(path)
+        dirname = os.path.normpath(dirname)
+        if basename == '__init__.py' and dirname in self.fake_package_cache:


A comment explaining what is going on and why?

- Remove dead code - Refactor getmtime() and abspath()/relpath() into BuildManager methods - Explain why we set tree.path (always setting it, I think that's fine) - Add docstrings to init_under_package_root() and _fake_init() - Add comments where fake_package_cache is checked - Refactor processing of package_root and cache_map options

gvanrossum · 2018-05-29T21:59:00Z

I think I've done everything you asked for, and I also merged master. Please re-review!

gvanrossum · 2018-06-02T02:55:47Z

I'm unclear about the test failures. I can't repro them locally, except once, and after that they passed. So they seem to be flakes, or the result of imperfect test isolation.

[UPDATE: Possibly I did a bad merge earlier, I don't see a passing test for this branch after 1f26bdc]

gvanrossum · 2018-06-03T05:16:47Z

The test flakes weren't due to a bad merge. In a recent commit I replaced an uncached os.path.getmtime() call with a call to fscache.stat() (and extracting the mtime). But that particular getmtime() call apparently needs to be uncached. This also (intuitively) explains why the failures were flakes -- they depended on filesystem state.

msullivan

Everything looking pretty good now!

thejcannon · 2022-07-22T17:27:37Z

For posterity "For files not in the cache map, the --cache-dir is still used." isn't true as of b0319af.

Just leaving it here because this cost me some headscratching 😄

gvanrossum mentioned this pull request Mar 19, 2018

Bazel build integration #3912

Open

Guido van Rossum added 10 commits March 19, 2018 13:41

New --bazel flag

13a52e1

Implement behavior changes for --bazel flag.

4618115

- Force relative paths. - Force mtimes to zero. - Locate cache files alongside source files.

Don't raise OSError for makedirs('')

d188e9f

Use zero mtimes in validate_meta() if bazel

9d32192

More zero mtime, and don't compare size -> always believe cache file …

f1c7834

…if present

Different tack for cache under Bazel: use --cache-dir but omit pyversion

aff2f72

Add --cache-map option

8048ee4

Pass cache mapping on command line

7bc3a0b

Tweak --cache-map arg parsing

3632c55

Drop the -i requirement for --bazel; it's not strictly needed

24de3d5

gvanrossum force-pushed the bazel-flag branch from 74e4f7c to 24de3d5 Compare March 19, 2018 20:43

Guido van Rossum and others added 2 commits March 20, 2018 13:37

Fix lint

ec719df

Somehow mypy/api.py had DOS line endings. Fix this. (python#4768)

fcc9604

sixolet reviewed Mar 22, 2018

View reviewed changes

Add docstring for relpath()

c62a22e

Guido van Rossum added 5 commits March 26, 2018 16:48

Merge remote-tracking branch 'upstream/master' into bazel-flag

72d2657

Monumental effort to support packages without __init__.py for Bazel (…

4dd90b8

…only)

Fix mypy errors

c667dfc

Fix tests

a43ecdc

Fix flake8

e17a702

Guido van Rossum added 6 commits April 4, 2018 10:43

Merge master (except typeshed; resolved one conflict in main.py)

0f4c8af

Merge branch 'master' into bazel-flag

cd10259

Make --package-root a separate flag

eade116

Merge master

e7ffae4

Sync typeshed (same as master)

f19892a

Add tests for --package-root and fix behavior

a0072ab

Guido van Rossum added 6 commits April 25, 2018 09:30

Merge master (some tests broken)

29f3f59

Change the way FileSystemCache gets the package_root

052fa08

Merge remote-tracking branch 'upstream/master' into bazel-flag

fb24a14

Avoid mypy error (see python/typeshed#2081)

05a448d

Merge master

8b88cd2

Fix fscache.py bug: only generate fake __init__.py, not .pyi

aa5facd

Guido van Rossum added 3 commits May 4, 2018 10:24

Merge master 211a654

755b540

Avoid updating hash in cache meta

7884ed4

Merge master (22ee713)

01f3893

Merge upstream/master (464b553)

1f26bdc

msullivan reviewed May 25, 2018

View reviewed changes

Guido van Rossum added 2 commits May 29, 2018 14:53

Merge master (0447473)

7559fc8

Guido van Rossum added 6 commits May 29, 2018 15:19

Don't force tree.path absolute in non-bazel -- it breaks Windows

210de9c

Add annotations to new functions, to make self-test pass

15b7d87

Hrmpf

4135e63

Merge master

6cbcb08

merge master (again)

52649f5

Fix bad merge

3bd329e

Hopefully fix flaky tests (don't use the fscache in getmtime())

389c908

msullivan approved these changes Jun 5, 2018

View reviewed changes

Merge master one last time

4665933

gvanrossum merged commit b78b621 into python:master Jun 5, 2018

gvanrossum deleted the bazel-flag branch June 5, 2018 22:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add several undocumented flags (--bazel, --package-root and --cache-map) in support of Bazel #4759

Add several undocumented flags (--bazel, --package-root and --cache-map) in support of Bazel #4759

gvanrossum commented Mar 19, 2018 •

edited

Loading

sixolet left a comment

sixolet Mar 22, 2018

sixolet Mar 22, 2018

gvanrossum Mar 22, 2018

sixolet Mar 22, 2018

gvanrossum Mar 22, 2018

kamahen Mar 22, 2018

sixolet Mar 22, 2018

gvanrossum commented Mar 26, 2018

gvanrossum commented Mar 29, 2018

sixolet commented Mar 30, 2018

gvanrossum commented May 2, 2018

gvanrossum commented May 22, 2018

msullivan left a comment

msullivan May 25, 2018

msullivan May 25, 2018

msullivan May 25, 2018

msullivan May 25, 2018

msullivan May 25, 2018

msullivan May 25, 2018

msullivan May 25, 2018

msullivan May 25, 2018

gvanrossum commented May 29, 2018 via email

gvanrossum commented Jun 2, 2018 •

edited

Loading

gvanrossum commented Jun 3, 2018

msullivan left a comment

thejcannon commented Jul 22, 2022

		@@ -71,6 +71,12 @@
		PYTHON_EXTENSIONS = ['.pyi', '.py']


		if os.altsep:

Add several undocumented flags (--bazel, --package-root and --cache-map) in support of Bazel #4759

Add several undocumented flags (--bazel, --package-root and --cache-map) in support of Bazel #4759

Conversation

gvanrossum commented Mar 19, 2018 • edited Loading

sixolet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum commented Mar 26, 2018

gvanrossum commented Mar 29, 2018

sixolet commented Mar 30, 2018

gvanrossum commented May 2, 2018

gvanrossum commented May 22, 2018

msullivan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum commented May 29, 2018 via email

gvanrossum commented Jun 2, 2018 • edited Loading

gvanrossum commented Jun 3, 2018

msullivan left a comment

Choose a reason for hiding this comment

thejcannon commented Jul 22, 2022

gvanrossum commented Mar 19, 2018 •

edited

Loading

gvanrossum commented Jun 2, 2018 •

edited

Loading