search

facebook / buck2

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

Add build info across all requests #502

Closed vaskomitanov closed 3 months ago

facebook-github-bot commented 6 months ago

Hi @vaskomitanov!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

facebook-github-bot commented 6 months ago

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

cormacrelf commented 6 months ago

There is already a type defined by RBE for this, which https://github.com/facebook/buck2/pull/310 sends. You could pick up that PR and add the config flag that was requested.

thoughtpolice commented 6 months ago

Yes, sorry for not getting around to it lately. I think #310 is the right way forward, but note that PR is very incomplete. Most of the plumbing for the RBEv2 metadata isn't there, so I think a lot of refactorings need to go in, first.

vaskomitanov commented 6 months ago

310 looks exactly what I needed, thanks for starting this effort @thoughtpolice. I will rebase my changes of yours and use the tool_invocation_id for this purpose.

ndmitchell commented 6 months ago

@vaskomitanov - are all your rebasings done now?

vaskomitanov commented 6 months ago

@vaskomitanov - are all your rebasings done now?

Yes. There are few fields not yet populated but even in this state we have all of the needed information for efficient scheduling for action execution and caching. I can add more information in a subsequent PR.

thoughtpolice commented 6 months ago

Thanks for taking this over the finish line! Please note that @krallin did I have one important adjustment he wanted me to make in https://github.com/facebook/buck2/pull/310#issuecomment-1612723781

Could we preferably expose this via a config option rather than a build flag? We do use our internal RE in order to test the OSS build, and that doesn't build with fbcode_build, so this would break that testing.

That is, they do a build of the OSS version of Buck internally and test that. So, we need to refactor this part of the diff in particular to just use a buckconfig value instead of conditional compilation thru #[cfg]:

https://github.com/facebook/buck2/pull/502/commits/2eaa19cdeab2c782e258cb455c05ce1db70a122f#diff-0bb786b03f4f35aef10055270239e2afbf4f4b0ba0fa214b38ffa858c4bc9e95R1257-R1323

Maybe we should call it:

[buck2_re_client]
use_fbcode_metadata = true

And then default it to false so OSS users don't have to even know about it.

vaskomitanov commented 6 months ago 
[buck2_re_client]
use_fbcode_metadata = true

And then default it to false so OSS users don't have to even know about it.

OK, sorry for the wait (thanksgiving), I've made requested changes. As a side note, to keep the current behavior, I've made use_fbcode_metadata= true. Can make it false by default if everybody thinks that should be the way to go.

Yashshiva75 commented 6 months ago

THIS IS A SAMPLE COMMENT

thoughtpolice commented 6 months ago

As a side note, to keep the current behavior, I've made use_fbcode_metadata= true. Can make it false by default if everybody thinks that should be the way to go.

IMO it should be turned off by default; after all, the code path in question is located inside the RPC client for the OSS Bazel API, so it's the exact path all OSS users rely on. This codepath should work as expected in OSS builds, i.e. with OSS metadata.

We could #[cfg] that choice, but I'm sure Meta can override it with a simple -c buck2_re_client.use_fbcode_metadata=true flag to buck2 in their test harness.

vaskomitanov commented 6 months ago

As a side note, to keep the current behavior, I've made use_fbcode_metadata= true. Can make it false by default if everybody thinks that should be the way to go.

IMO it should be turned off by default; after all, the code path in question is located inside the RPC client for the OSS Bazel API, so it's the exact path all OSS users rely on. This codepath should work as expected in OSS builds, i.e. with OSS metadata.

We could #[cfg] that choice, but I'm sure Meta can override it with a simple -c buck2_re_client.use_fbcode_metadata=true flag to buck2 in their test harness.

OK, done, default is "false"

vaskomitanov commented 6 months ago

@ndmitchell any additional steps I need to do to get this PR merged?

ndmitchell commented 5 months ago

Sorry for the delay - was travelling then got a bit ill. Just running through the code now.

facebook-github-bot commented 3 months ago

@JakobDegen has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot commented 3 months ago

This pull request has been reverted by 1d39ef8c4306fc18539388c56edc2835a75ab654.

JakobDegen commented 2 months ago

The label was technically right that this was reverted, but I relanded it a day later so all good :)