-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Track empty directories with BAZEL_TRACK_SOURCE_DIRECTORIES #15774
Conversation
a657740
to
389682a
Compare
@lberki Is this also something you feel comfortable reviewing? I'm looking into |
Our current plan is to completely remove support for source directories 1, so I'd prefer we didn't make any changes to We are going to do this at relatively low priority. If you (@fmeum) or any Bazel stakeholders don't like that, let us know and we'll reconsider. Otherwise I'd prefer rejecting this PR or at least putting it on hold until @lberki and I discuss further. Footnotes |
@haxorz This is definitely unexpected. Could you elaborate a bit more on why you are planning to do this? As far as I understand the situation, having proper source directory tracking has at least two distinct advantages compared to globs:
If it's just an issue of priorities/staffing, I'm happy to contribute more extensive tests for the feature. |
The decision to research removing support for source directories was made in ~Dec 2021. The outcome of this research may be to remove the feature. It may also be to go in the other direction and make I didn't want to prematurely commit to the latter outcome right now by approving this PR today.
Yes, great points. Both of those were brought up as theoretical concerns in the discussion in Dec 2021 (comments 9-12 & 32-35 in Google internal issue 7075837). Researching whether they are concerns in practice, both for Google's internal codebase, and for Bazel users everywhere, is still an open task.
In particular, I personally agree with you. But (a) there was not full agreement on this in Dec 2021 and (b) I want to leave this decision up to the assignee of the research task (currently @alexjski, who is OOO for the next few weeks).
Thanks for the offer. @alexjski , wdyt? Should we just go all-in on |
@haxorz Thanks for the clarifications, I now have a much better understanding of the situation. Any chance you could make the information on that Google-internal ticket accessible to me or even public? |
I'l be out of office for a while so I probably shouldn't express opinions, so I'll just have an observation: approving this pull request would in no way mean committing to the |
My current thinking is that I will try as hard as I can to remove that mostly to remove complexity. That is not to say that this is a done deal, but my approach is that unless I see strong evidence that it is necessary to support this feature and it is the best way to support given usage cases, I will try to cut it. I think of this as an opportunity to reevaluate this feature.
This is a great point, definitely something to keep in mind. My hope is that a push to remove that internally would show us whether the problems really exist. I would like to stress out that there is some complexity to handle here, namely when detecting changes locally. If we are to support the directories, we would need to implement that for source directories properly and maybe make that work with things like Now, let's imagine we implement support for source directories, if it uses O(files) structure in Skyframe, then we could probably consider that not better than globs. Example is if we depended on
For this one, I wonder how much this is the case of "this use case is best solved by source directories" vs "this use case happens to work with them". One thing which comes to my mind is tree artifacts which do allow for e.g. I can see couple categories of problems here (not sure which ones do you run into):
There is obviously a problem of staffing, but to me, we also have an issue of complexity. One more problem to mention is remote execution which actually needs enumerated file inputs, leaving the need to readdir and digest the files (again?) at execution time. This is something that globs do not suffer from. We could make source directories internally equivalent to globs, in which case the feature becomes more of a convenience shorthand. In order to recommend that, I would need to understand the use cases which really need that better. |
Well, that might be easy to answer. The reason I added As for simplification, I don't recall Changing the label syntax is unlikely to be a viable option, IMO, but - of course - if there's a way to do that, that would be good. |
Nice, so that means that this was added with that particular goal in mind. Would you mind expanding to why we didn't use tree artifacts for that instead? I understand that would be less convenient to use, but wouldn't require a feature.
I gave that a brief thought, on the surface, that seems relatively simple--we could e.g. have escaping like SQL does for |
src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalValue.java
Outdated
Show resolved
Hide resolved
57a7f27
to
8903f0d
Compare
Update on this--I did some more research which leads me to believe that this statement about iOS is no longer true. It is however true that internally there are files which have Anyway, I believe that adding full support for source directories will be quite some work, which is why tree artifacts hack is so tempting (those do work already). At this point, I would refrain from making any promises going either way. |
Do you have any expectations regarding the next steps? Are there any obvious issues with source directory tracking that would need to be resolved or is it more of a hunt for edge cases? |
I think that this discussion is orthogonal to this PR. This PR purely improves the behavior of |
src/test/java/com/google/devtools/build/lib/skyframe/TrackSourceDirectoriesIntegrationTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/google/devtools/build/lib/skyframe/TrackSourceDirectoriesIntegrationTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/google/devtools/build/lib/skyframe/TrackSourceDirectoriesIntegrationTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/google/devtools/build/lib/skyframe/TrackSourceDirectoriesIntegrationTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/google/devtools/build/lib/skyframe/TrackSourceDirectoriesIntegrationTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/google/devtools/build/lib/skyframe/TrackSourceDirectoriesIntegrationTest.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalValue.java
Outdated
Show resolved
Hide resolved
8903f0d
to
d90f167
Compare
...st/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
Outdated
Show resolved
Hide resolved
d90f167
to
5d25731
Compare
src/test/java/com/google/devtools/build/lib/skyframe/TrackSourceDirectoriesIntegrationTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/google/devtools/build/lib/skyframe/TrackSourceDirectoriesIntegrationTest.java
Show resolved
Hide resolved
src/test/java/com/google/devtools/build/lib/skyframe/TrackSourceDirectoriesIntegrationTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/google/devtools/build/lib/skyframe/TrackSourceDirectoriesIntegrationTest.java
Show resolved
Hide resolved
src/test/java/com/google/devtools/build/lib/skyframe/TrackSourceDirectoriesIntegrationTest.java
Outdated
Show resolved
Hide resolved
...st/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
Outdated
Show resolved
Hide resolved
...st/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
Show resolved
Hide resolved
...st/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
Show resolved
Hide resolved
8eebf75
to
ced7e8a
Compare
ced7e8a
to
d1894c9
Compare
@alexjski Friendly ping. I resolved the merge conflict. |
...st/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/google/devtools/build/lib/skyframe/TrackSourceDirectoriesIntegrationTest.java
Show resolved
Hide resolved
Sorry, it fell off my radar. Looking pretty good, added only some minor comments. |
RecursiveFilesystemTraversalFunction did not emit a ResolvedFile for empty directories and thus didn't track their existence at all since they have no children that would track it implicitly. This commit adds the tracking of empty directories for DirectoryArtifactTraversalRequest only, which is used to implement the BAZEL_TRACK_SOURCE_DIRECTORIES flag. Also adds a basic test for the source directory tracking functionality.
d1894c9
to
bd52176
Compare
...st/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
Show resolved
Hide resolved
src/test/java/com/google/devtools/build/lib/skyframe/TrackSourceDirectoriesIntegrationTest.java
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/skyframe/TraversalRequest.java
Show resolved
Hide resolved
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.
Thank you for fixing that!
RecursiveFilesystemTraversalFunction did not emit a ResolvedFile for empty directories and thus didn't track their existence at all since they have no children that would track it implicitly. This commit adds the tracking of empty directories for DirectoryArtifactTraversalRequest only, which is used to implement the BAZEL_TRACK_SOURCE_DIRECTORIES flag. Also adds a basic test for the source directory tracking functionality. Closes bazelbuild#15774. PiperOrigin-RevId: 468804794 Change-Id: I2c06c05b335781234d4fa90f3d8c1e7fe6dd184a
RecursiveFilesystemTraversalFunction did not emit a ResolvedFile for
empty directories and thus didn't track their existence at all since
they have no children that would track it implicitly.
This commit adds the tracking of empty directories for
DirectoryArtifactTraversalRequest only, which is used to implement the
BAZEL_TRACK_SOURCE_DIRECTORIES flag.
Also adds a basic test for the source directory tracking functionality.