-
-
Notifications
You must be signed in to change notification settings - Fork 623
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
Make "changed" tasks work with deleted files #4546
Make "changed" tasks work with deleted files #4546
Conversation
|
||
def glob_to_regex(pattern): | ||
"""Given a glob pattern, return an equivalent regex expression. | ||
:param string glob: The glob pattern. "**" matches 0 or more dirs recursively. |
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.
this should already be provided by the pathspec
3rdparty dep?
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.
glob syntax used in file matching is not equivalent to gitignore syntax. For example, "src" in gitignore will match everything inside "src", but in glob syntax, it should only match "src".
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.
Thanks Yujie.
@@ -48,20 +49,27 @@ def _unique_dirs_for_sources(self, sources): | |||
def target_addresses_for_source(self, source): | |||
return list(self.iter_target_addresses_for_sources([source])) | |||
|
|||
def _iter_owned_files_from_hydrated_target(self, legacy_target): | |||
def _match_source(self, source, fileset): | |||
if os.path.exists(source): |
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.
Rather than always checking whether it exists, maybe first check whether it is in the fileset... if it is, then it's definitely owned, and that's cheaper than a syscall. If it's not in the fileset, can use matches_filespec
.
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.
done.
else: | ||
return matches_filespec(source, fileset.filespec) | ||
|
||
def _own_source(self, source, legacy_target): | ||
"""Given a `HydratedTarget` instance, yield all files owned by the target.""" |
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.
Comment out of date. I think this method is probably _owns_source
since it returns a boolean?
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.
done.
Problem
#4529
There are 2 kinds of "changed" tasks in pants currently. Neither of them output correct result if a file is deleted.
Solution
I restored the removed file matching logic (using regular expression). In both SpecSourceMapper and EngineSourceMapper, first check if a file exists. It it exists, then check if it's in target's file set. If not, then it is a deleted file, and use re matching logic on it. Fixes #4529