facebook / buck2

Build system, successor to Buck
https://buck2.build/
Apache License 2.0
3.6k stars 223 forks source link

Should `allow_cache_upload` be available on more rules? #478

Open cormacrelf opened 1 year ago

cormacrelf commented 1 year ago

As far as I can tell, the allow_cache_upload flag is available only on 3 specific binary rules, and nowhere else. allow_cache_upload is also default False on every actions.run() call, so there are probably a hundred places in prelude that have yet to get it threaded through. If that was ever the goal.

Available:

Not available, for example:

There's obviously some rhyme to this, in that somebody committed those three | buck.allow_cache_upload_arg() attributes only on binary rules, but I can't find a rationale. I have a few guesses:

I just quickly patched prelude to enable it on rust_library and supply it in the macro reindeer calls for all crates.io targets, and it seems to work great. You can launch another buckd on a separate isolation dir on a big crate graph that reindeer made, and it eats it for breakfast.

cormacrelf commented 1 year ago

Ah, I think I see what's going on. allow_cache_upload is getting slowly rolled out to more rules as the responsible team becomes sure they've dealt with the issues. https://github.com/facebook/buck2/commit/211d555e71aa80f78cd037c8580e99a20a8990d6

krallin commented 1 year ago

The default is that actions executed on RE get cached there implicitly, this flag only controls behaviour when actions get executed locally

Internally the majority of actions execute on RE, so this flag only makes sense on things that may disable running on RE in the first place.

That said, the rules you see it on are typically rules where cache misses were found to be problems on things that were forced to execute locally.

We’re not really fans of how this behaviour is configured; it’s OK but it’s clearly a local optimum.

On Sun, 5 Nov 2023 at 15:20, Cormac Relf @.***> wrote:

Ah, I think I see what's going on. allow_cache_upload is getting slowly rolled out to more rules as the responsible team becomes sure they've dealt with the issues. 211d555 https://github.com/facebook/buck2/commit/211d555e71aa80f78cd037c8580e99a20a8990d6

— Reply to this email directly, view it on GitHub https://github.com/facebook/buck2/issues/478#issuecomment-1793751275, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANIHVR7UXK5TNLI3OCCREDYC6OELAVCNFSM6AAAAAA66LFYDCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJTG42TCMRXGU . You are receiving this because you are subscribed to this thread.Message ID: @.***>

cormacrelf commented 1 year ago

Outside of an RE-majority context, choosing to allow cache uploads seems to be more a function of the hermeticity of toolchains, rather than something you'd want to be forced to enable and disable on individual rules. I agree the default should probably be off on the system_*/demo toolchains that are probably not going to cache well. But other toolchains can default to true.

Then I could configure my rust toolchain to allow cache uploads, and the full crate graph would start flowing into the cache.

Rust_library et al would read the toolchain's value for allow_cache_upload. You can keep a flag on those individual rules as an override, treating it as an optional bool with the default value as None. Whatever works.

ndmitchell commented 12 months ago

I wonder if the more pragmatic thing is to have allow_cache_upload default to None, where True means yes, False means no, and None means you consult some config. We'd leave it off at Meta, but if someone outside Meta wants to cause everything to upload I see no reason not to make that easy.