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

Empty directories inside tree artifacts are not tracked #15901

Open
tjgq opened this issue Jul 18, 2022 · 3 comments · May be fixed by #16015
Open

Empty directories inside tree artifacts are not tracked #15901

tjgq opened this issue Jul 18, 2022 · 3 comments · May be fixed by #16015
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Core Skyframe, bazel query, BEP, options parsing, bazelrc team-Remote-Exec Issues and PRs for the Execution (Remote) team type: feature request

Comments

@tjgq
Copy link
Contributor

tjgq commented Jul 18, 2022

Description of the bug:

Skyframe doesn't track the existence of empty directories inside a tree artifact. As a result, if a tree artifact with an empty directory is produced and the empty directory is manually deleted, rebuilding doesn't recreate the empty directory. Likewise, changing a rule implementation to produce an empty directory when it previously didn't doesn't invalidate the build results.

I believe the root cause is that we model a TreeArtifactValue (the SkyValue representing the contents of a tree artifact) as a collection of TreeFileArtifacts, one per regular file inside the tree artifact, ignoring empty directories.

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

Workspace:

cat > BUILD <<'EOF'
load(":defs.bzl", "my_rule")

my_rule(name = "my")
EOF

touch WORKSPACE

cat > defs.bzl <<'EOF'
def _impl(ctx):
  d = ctx.actions.declare_directory(ctx.label.name + ".dir")
  ctx.actions.run_shell(
    outputs = [d],
    command = "cd %s && mkdir empty" % d.path,
  )
  return DefaultInfo(files = depset([d]))

my_rule = rule(implementation = _impl)
EOF

Then run:

bazel build :my  # clean build
ls bazel-bin/my.dir  # contains empty subdirectory
chmod u+w bazel-bin/my.dir && rmdir bazel-bin/my.dir/empty
bazel build :my  # incremental build
ls bazel-bin/my.dir  # should contain empty subdirectory, but doesn't

Which operating system are you running Bazel on?

Linux, MacOS

What is the output of bazel info release?

development version

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

Built @ 6f0913e (near current head)

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

N/A

Have you found anything relevant by searching the web?

N/A

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

N/A

@fmeum
Copy link
Collaborator

fmeum commented Jul 18, 2022

@tjgq What do you think of #15789 (comment)? I could work on this, I always felt like #15276 wasn't as clean as it could have been due to this missing.

@tjgq
Copy link
Contributor Author

tjgq commented Jul 18, 2022

@fmeum Yes, I think you are correct and we either have to extend the definition of TreeFileArtifact to also cover empty directories, or introduce a new artifact type (TreeDirectoryArtifact?) for them. Then we should be able to track the existence of empty subdirectories correctly in Skyframe. Feel free to work on this.

@sgowroji sgowroji added type: bug untriaged team-Remote-Exec Issues and PRs for the Execution (Remote) team labels Jul 19, 2022
@fmeum fmeum linked a pull request Aug 1, 2022 that will close this issue
@vladmos vladmos removed the untriaged label Aug 2, 2022
@coeuvre coeuvre added the P2 We'll consider working on this in future. (Assignee optional) label Nov 22, 2022
@tjgq tjgq removed their assignment Oct 16, 2023
@tjgq
Copy link
Contributor Author

tjgq commented Oct 16, 2023

My previous comment was a little too optimistic. I do think there are use cases where accurate tracking of tree artifact contents is required, and this probably goes beyond empty subdirectories (for example, it's also impossible for a tree artifact to contain a symlink since we always dereference them; see #15454). But this would be a feature request, not a bug.

I'm reassigning this to the core team because I feel that's where a decision to implement belongs. (I'm not asking for a decision to be made at this time, though.)

@tjgq tjgq added team-Core Skyframe, bazel query, BEP, options parsing, bazelrc P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed P2 We'll consider working on this in future. (Assignee optional) labels Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Core Skyframe, bazel query, BEP, options parsing, bazelrc team-Remote-Exec Issues and PRs for the Execution (Remote) team type: feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants