Skip to content
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

Zipapps no longer deleting Bazel.runfiles_* directories placed in /tmp #17342

Closed
sethasadi opened this issue Jan 26, 2023 · 10 comments · May be fixed by #17628
Closed

Zipapps no longer deleting Bazel.runfiles_* directories placed in /tmp #17342

sethasadi opened this issue Jan 26, 2023 · 10 comments · May be fixed by #17628
Assignees
Labels
team-Rules-Python Native rules for Python type: bug

Comments

@sethasadi
Copy link

Description of the bug:

As of Bazel 6.0.0, a change was made to bazel/rules/python/python_stub_template.txt that gates the deletion of the Bazel.runfiles_* directory that is created in /tmp. This is because it depends on workspace being set, and workspace is only set when RUN_UNDER_RUNFILES is set. This is a regression from the previous behavior where the directory placed in /tmp would be deleted as long as it was running under a zip. This should be fixed so that the Bazel.runfiles_* directories are cleaned up whenever it is running under a zip.

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Build a zipapp target on Bazel 6.0.0 (without RUN_UNDER_RUNFILES set) and on whatever machine you consume that zipapp there will be a Bazel.runfiles_<sha> directory with the zipapp contents left around from execution.

Which operating system are you running Bazel on?

Linux

What is the output of bazel info release?

release 6.0.0

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

@sethasadi
Copy link
Author

Pinging this issue. @rickeylev I noticed you've been making changes around the Python rules here. Would you be able to take a look? This issue is coming up more and more as more projects are upgraded to Bazel 6.

@rickeylev rickeylev self-assigned this Feb 28, 2023
@rickeylev
Copy link
Contributor

rickeylev commented Feb 28, 2023

What is this "RUN_UNDER_RUNFILES" environment variable? Is there some doc about this? As near as I can tell it's only set when Bazel runs a target as a test?

Anyways, yeah, bug. I was able to repro.

Posterity/notes to self: This was PR #15590

rickeylev added a commit to rickeylev/bazel that referenced this issue Feb 28, 2023
…ries.

PR bazelbuild#15590 accidentally introduced a bug where zip-based binaries weren't
cleaning up the temporary runfiles directory they extracted their zip
into when they were run *outside* of a test invocation.

To fix, explicitly keep track of when the module space needs to be
deleted or not, and pass that long to the code that decides how to
execute the program and how to clean it up afterwards.

Fixes bazelbuild#17342
@sethasadi
Copy link
Author

@rickeylev thank you for getting this done so quickly. Can you merge this into the Bazel 6.1 branch so that it can go out as soon as possible?

@rickeylev
Copy link
Contributor

I suppose so. I've never done or requested a backport, so I'll have to ask around how to do that.

@sethasadi
Copy link
Author

@rickeylev this mentions how: #17212 (comment)

@keertk
Copy link
Member

keertk commented Mar 1, 2023

Hi all, can this wait till the next release (6.2)? We've already created the third release candidate for 6.1 and plan to do the final release on Monday, so we won't be able to include this.

@rickeylev
Copy link
Contributor

Hi,

Yes, if it's too late for 6.1, then 6.2 is fine.

@keertk keertk removed the untriaged label Mar 2, 2023
@keertk
Copy link
Member

keertk commented Mar 2, 2023

@bazel-io fork 6.2.0

ShreeM01 pushed a commit to ShreeM01/bazel that referenced this issue Mar 13, 2023
PR bazelbuild#15590 accidentally introduced a bug where zip-based binaries weren't
cleaning up the temporary runfiles directory they extracted their zip into when
they were run outside of a test invocation.

To fix, explicitly keep track of when the module space needs to be deleted or
not, and pass that long to the code that decides how to execute the program and
how to clean it up afterwards.

Fixes bazelbuild#17342

PiperOrigin-RevId: 513256904
Change-Id: I8c3d322248f734a6290a8a7ee5c8725fae5203dc
ShreeM01 added a commit that referenced this issue Mar 13, 2023
…17764)

PR #15590 accidentally introduced a bug where zip-based binaries weren't
cleaning up the temporary runfiles directory they extracted their zip into when
they were run outside of a test invocation.

To fix, explicitly keep track of when the module space needs to be deleted or
not, and pass that long to the code that decides how to execute the program and
how to clean it up afterwards.

Fixes #17342

PiperOrigin-RevId: 513256904
Change-Id: I8c3d322248f734a6290a8a7ee5c8725fae5203dc

Co-authored-by: Richard Levasseur <[email protected]>
fweikert pushed a commit to fweikert/bazel that referenced this issue May 25, 2023
PR bazelbuild#15590 accidentally introduced a bug where zip-based binaries weren't
cleaning up the temporary runfiles directory they extracted their zip into when
they were run outside of a test invocation.

To fix, explicitly keep track of when the module space needs to be deleted or
not, and pass that long to the code that decides how to execute the program and
how to clean it up afterwards.

Fixes bazelbuild#17342

PiperOrigin-RevId: 513256904
Change-Id: I8c3d322248f734a6290a8a7ee5c8725fae5203dc
@mpereira
Copy link

I was seeing this on 6.3.2 and tried upgrading to 7.1.1 to see if it fixed it, but it still happens.

Anyone else still seeing this?

@jschmitz-ssg
Copy link

I also still see this in 6.2.1. I haven't tried a newer version. The runfiles dir is cleaned up if the subprocess exits normally, but if the process is signaled, the shutil.rmtree never gets executed.

I don't know how this might affect non-zipapp use cases, but maybe register a sighandler for TERM/INT to signal the child process, then reset the disposition. Otherwise, to try and use Python's tempfile.TemporaryDirectory to have the interpreter do a best-effort cleanup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Rules-Python Native rules for Python type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants