Open izissise opened 10 months ago
Yeah, I've noticed this. I think Please checks target outputs when building a target: https://github.com/thought-machine/please/blob/master/src/build/incrementality.go#L84
I don't think this picks this case up though, unfortunately.The path is a directory and the directory still exists. We'd have to walk the dir tree and check the hash sum against the expected hash sum for that dir or something. I think this could have a performance impact if the directory tree is large.
I've been saying that modifying the targets in the subrepos directory is a bad idea, and people should just not do that, but I've also had a few engineers internally have those files mysteriously modified. I think this can bite you in subtle and hard to debug ways, so maybe we should try and figure out if there's something we can do here.
It seems to me, that subrepos should check if the rule declared as dep
needsBuilding
I think it does, unless I'm mistaken. We queue the subrepo target up to be built just like normal, when we hit Parse()
for the subrepo label.
I think I have an idea for what we can do here, that would bring local execution closer to how remote execution works. With remote execution, we're changing the way subrepo outputs are handled, which will actually resolve this issue. I think we might be able to do something similar with local execution.
With remote execution, we have two caches. One is content addressable, and the other is an action cache keyed by the action digest (which essentially equates to the same thing as the rule hash returned by plz query hash
). We build up the rule hash which can be determined locally from repo state. From where we can fetch the action result from the action cache, which contains, amongst other things, a merkle tree representing the output directory of the action. We can use this to fetch the outputs from the content addressable storage (CAS).
We're aiming to take advantage of this for remote execution, so that we don't download subrepos to plz-out
at all. We instead load things from the CAS (caching to disk for performance) directly, so files can't be modified locally (unless you go edit things under ~/.cache/please
).
We could change the way the local directory cache works, so it's content addressable in the same way. We would have a target cache that maps the rule's hash to the output merkle tree. This merkle tree could would then load the BUILD file or .build_defs
files from the CAS based on the recorded output hashes, so if the files changed in plz-out
this would no longer affect parsing. In theory, we could use the same directory protos locally as we do remotely.
I think this is a fairly chunky bit of work but is probably worth doing. What do you think @peterebden ?
Retaking the example repo https://github.com/Wuageorg/please_issue_2943
the dependency graph is
$ please query deps //dir1
//dir1:dir1
//dir1:subrepo
///dir1/this_subrepo//:build
Subrepos could implies dependencies on their declared dep
. In this case this would mean the subrepo ///dir1/this_subrepo
would add a dependency on //dir1:subr_srcs
triggering a rebuild of the subrepo when script.sh
changes since it is in srcs
of //dir1:subr_srcs
.
Hence the graph for the example would become
$ please query deps //dir1
//dir1:dir1
//dir1:subrepo
///dir1/this_subrepo//:build
//dir1:subr_srcs
//dir1:subr_buildfile
Maybe the operation is not clear, I get this bug when modifying the source file dir1/script.sh
and not the one inside the subrepo.
It does trigger a rebuild for //dir1:subr_srcs
build_rule, but does not rebuild the subrepo tree.
That's reason I believe there is a missing dependency
Example repo -> https://github.com/Wuageorg/please_issue_2943
command
please build && bash plz-out/bin/dir1/this_subrepo/bin.sh
It will display
as intended the first time, now modify
dir1/script.sh
as indicated and redoplease build && bash plz-out/bin/dir1/this_subrepo/bin.sh
it will still displayfull output
the BUILD file https://github.com/Wuageorg/please_issue_2943/blob/main/dir1/BUILD does something akin to go-rule's go_repo https://github.com/please-build/go-rules/blob/master/build_defs/go.build_defs#L1150