fixes #28183
The custom cache dir for mypy can only be set via an environment variable, setting the MYPY_CACHE_DIR variable in the program is not sufficient. This error was introduced while translating the shell script to python.
See also the mypy documentation: https://mypy.readthedocs.io/en/stable/config_file.html#confval-cache_dir
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | MarcoFalke |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
12 | @@ -13,8 +13,10 @@ 13 | import subprocess 14 | import sys 15 | 16 | +# Customize mypy cache dir via environment variable
Why is this needed?
If it is not needed, it may be better to remove it.
This was added as part of #18210 but I didn't see an explanation given there. I think it is better to specify a location explicitly since the default is using the current directory which may be leading to unexpected results if this script is called from different contexts. But if we don't care about something like this, it may be removed.
12 | @@ -13,8 +13,10 @@ 13 | import subprocess 14 | import sys 15 | 16 | +# Customize mypy cache dir via environment variable 17 | +os.environ["MYPY_CACHE_DIR"] = f"{os.getenv('BASE_ROOT_DIR', '')}/test/.mypy_cache"
This will write to /test/ if a user is running this script:
# ./test/lint/lint-python.py
Success: no issues found in 269 source files
# ls /test/.mypy_cache/
3.11 CACHEDIR.TAG
That was the previous behavior. There is even an explicit line for this in our .gitignore. What would you like to do instead? Move it into /test/cache/?
No, I mean /test/, not ./test/.
With /test being next to /bin, etc. I don't think .gitignore will work on the literal root of your filesystem. .gitignore should be limited to the folder it is located in.
I see, I simply misread. I changed this to get the base path relative to file as a fallback. We could also simply use . or fall back to not setting a cache_dir at all but this seemed the most robust option to me.
I see, I simply misread. I changed this to get the base path relative to file as a fallback. We could also simply use
.or fall back to not setting acache_dirat all but this seemed the most robust option to me.
Wouldn't the default option result in placing the cache dir in the root, regardless of where the script is being run from?
left some questions
Are you still working on this? If not, could close, so that someone else can pick it up?
12 | @@ -13,8 +13,11 @@ 13 | import subprocess 14 | import sys 15 | 16 | +# Customize mypy cache dir via environment variable 17 | +base_dir = os.getenv('BASE_ROOT_DIR', os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))))
base_dir = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
The two should be the same, no? Seems better to just use one way to get one thing, instead of two.
If BASE_ROOT_DIR isn't used differently yes, I wasn't 100% sure about that. Done.
12 | @@ -13,8 +13,11 @@ 13 | import subprocess 14 | import sys 15 | 16 | +# Customize mypy cache dir via environment variable 17 | +base_dir = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
nit: Maybe use https://docs.python.org/3.8/library/pathlib.html#pathlib.Path.resolve for new code? And .parent instead of dirname? And operator/ over manual string formatting below?
done
Addressed feedback by @MarcoFalke and rebased
15 | 16 | from importlib.metadata import metadata, PackageNotFoundError 17 | 18 | +# Customize mypy cache dir via environment variable 19 | +base_dir = Path(__file__).parent.parent.parent 20 | +cache_dir = base_dir / "test" / ".mypy_cache"
cache_dir = Path(__file__).parent.parent / ".mypy_cache"
done
lgtm, left a nit
lgtm ACK f9047771d642c5887c752872b6ffbbd974603b35