-
Notifications
You must be signed in to change notification settings - Fork 70
profilers/python_ebpf.py: specifying newly TMPDIR on each instance of PyPerf #921
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
gprofiler/profilers/python_ebpf.py
Outdated
| self._pyperf_staticx_tmpdir: Optional[Path] = None | ||
| if os.environ.get("TMPDIR", None) is not None: | ||
| # We want to create a new level of hirerachy in our current staticx tempdir. | ||
| self._pyperf_staticx_tmpdir = Path(os.environ["TMPDIR"]) / random_prefix() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| self._pyperf_staticx_tmpdir = Path(os.environ["TMPDIR"]) / random_prefix() | |
| self._pyperf_staticx_tmpdir = Path(os.environ["TMPDIR"]) / "pyperf_" + random_prefix() |
|
@d3dave are you able to complete the review this week? |
CI is red |
|
Merged the CI fix from |
| # ensure `TMPDIR` env is propagated to the child processes (used by staticx) | ||
| if "TMPDIR" not in env and "TMPDIR" in os.environ: | ||
| if tmpdir is not None: | ||
| tmpdir.mkdir(exist_ok=True) | ||
| env["TMPDIR"] = tmpdir.as_posix() | ||
| elif "TMPDIR" not in env and "TMPDIR" in os.environ: | ||
| # ensure `TMPDIR` env is propagated to the child processes (used by staticx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure this is the right approach
maybe should we use tmpdir always relative to the current env[“TMPDIR”] (e.g. f”{env[“TMPDIR”]}/{tmpdir}”)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a practical difference? Doesn't the current flow always create the tmpdir passed to this function under $TMPDIR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@d3dave current approach:
if tmpdir is set - mkdir it (w/o parents). propagate it to the child.
suggested approach:
if tmpdir is set - mkdir "$TMPDIR/tmpdir“. propagate it to the child.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but the tmpdir arg, whenever it is passed it is always Path(os.environ["TMPDIR"]) / ("pyperf_" + random_prefix()) which is under $TMPDIR. So there is no difference because with both approaches the tmpdir is created under $TMPDIR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there’s no practical differences… I raised this to discuss what’s the better approach
Description
cleanup each instance of PyPerf staticx directories on termination.
Related Issue
We experienced where there were a-lot of staticx tempdirs leftovers.
Motivation and Context
How Has This Been Tested?
Screenshots
Checklist: