#general
- g
gorgeous-fish-11040
08/10/2015, 6:30 PMAlso fixed a bug in what_changed to respect removed files: https://rbcommons.com/s/twitter/r/2589/ - w
witty-crayon-22786
08/10/2015, 6:32 PMwhoa, is that what that is?!? should mention that in the description - g
gorgeous-fish-11040
08/10/2015, 7:01 PM@witty-crayon-22786: fixed summary simple smile - g
gorgeous-fish-11040
08/10/2015, 7:02 PMnext step will be to fix it for removed targets. See https://github.com/pantsbuild/pants/issues/382 - b
bored-art-40741
08/10/2015, 7:05 PM@gorgeous-fish-11040: does that change remove the laziness of
?LazySourceMapper - b
bored-art-40741
08/10/2015, 7:05 PMIf so, why? - g
gorgeous-fish-11040
08/10/2015, 7:08 PMyep. it was lazy because it was creating huge maps from a file to a target. after my change it doesn’t use files at all. Instead it looks at specs(glob(‘*.java’)) and tries to match them to changed files. - g
gorgeous-fish-11040
08/10/2015, 7:08 PMat least it’s my understanding why it was lazy. simple smile - g
gorgeous-fish-11040
08/10/2015, 7:09 PMnow it’s a walk through parent directories + checking for BUILD files from cache - b
bored-art-40741
08/10/2015, 7:10 PMIt is not clear to me that this is a safe change - b
bored-art-40741
08/10/2015, 7:10 PMWe use
quite a bit within buildgenLazySourceMapper - b
bored-art-40741
08/10/2015, 7:11 PMAnd semantically, asking the question "does this path match this glob's pattern" is different from "does this target actually own this path" - b
bored-art-40741
08/10/2015, 7:11 PMThe latter is not true when the path isn't on disk, and that distinction can be important - g
gorgeous-fish-11040
08/10/2015, 7:13 PMbut this is the case! if a file was removed it will appear in vcs diff but the previous logic was ignoring them - b
bored-art-40741
08/10/2015, 7:14 PMCorrect, I see why it's a bug in changed-target-detection, and I agree that this is the right general strategy to fix it (remember the patterns the target uses to match sources) - b
bored-art-40741
08/10/2015, 7:14 PMWhat I'm saying is that LazySourceMapper isn't only used for changed-target-detection, and it's dangerous to change the semantics of it - b
bored-art-40741
08/10/2015, 7:16 PMI'd prefer to see the shared functionality of
and the other more specificLazySourceMapper
(or whatever) split into a superclass or utility functions, and for the concepts to remain separateTargetSourcePatternMapper - g
gorgeous-fish-11040
08/10/2015, 7:19 PMThere is only one usage of the mapper in
which is used in several goals likeChangedFileTaskMixin
,changed
andcompile-changed
. But they all should be consistent.test-changed - b
bored-art-40741
08/10/2015, 7:20 PMAs I said, it's used in buildgen internally at Foursquare - b
bored-art-40741
08/10/2015, 7:20 PMIndeed that was its original use and why I originally wrote it, but we ended up using it for the rewrite of the changed goals because of its generality - b
bored-art-40741
08/10/2015, 7:22 PMHowever let me check the buildgen code more carefully - b
bored-art-40741
08/10/2015, 7:22 PMThis might be safe, I'm just not sure yet - g
gorgeous-fish-11040
08/10/2015, 7:22 PMRight. But if only the new implementation will be used in OSS Pants why keep the second implementation. Or you want to have a flag to choose between implementations? - b
bored-art-40741
08/10/2015, 7:23 PMOSS pants isn't the only consumer, that's what I'm saying - b
bored-art-40741
08/10/2015, 7:23 PMBreaking semantics or interfaces of public, generic utilities takes care because of this particular situation - w
witty-crayon-22786
08/10/2015, 7:24 PMwhat makes it a public interface? - b
bored-art-40741
08/10/2015, 7:24 PMsrc/python/pants/base/lazy_source_mapper.py - b
bored-art-40741
08/10/2015, 7:25 PMLiving in
, mostlypants.base - b
bored-art-40741
08/10/2015, 7:25 PMAnd the fact that it's well documented as a "utility". - g
gorgeous-fish-11040
08/10/2015, 7:25 PMRight. Right. But API is the same. It returns changed files or targets. But now it will respect removed files as well.