Skip to content
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

Add a --build flag to buck2 test #702

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cbarrete
Copy link
Contributor

No description provided.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 21, 2024
@cbarrete
Copy link
Contributor Author

@ndmitchell @JakobDegen Hello, would you mind having a look when you have a moment?
Thanks a lot!

@JakobDegen
Copy link
Contributor

Hm, interesting, I assumed we were already doing this. Tbh I'm half of the mind that this should be the default behavior and we shouldn't even support the current thing, but I have no idea what the history on this choice is and if others agree with me. I'll make a post in our internal discussion group to try and a few more eyes on this issue (will direct discussion over here of course)

@cbarrete
Copy link
Contributor Author

Thanks, I look forward to hearing from this!

I don't know about the history either, but I can see the current behavior making sense as well, as a target pattern which matches expensive targets (e.g. binaries that pull in a lot of targets and/or are heavily templated; packages/release tarballs/OCI images/etc.) would cause a "simple" buck2 test to be surprisingly expensive, but I personally wouldn't mind making this the default (potentially with a flag to opt out/only build what is necessary for tests).

@JakobDegen
Copy link
Contributor

One additional thought: If we do go the direction of a flag, we should probably use the same --build-{default,run}-info flags that buck2 build uses, instead of coming up with something new. But let's agree on whether we should have a flag at all before getting into that.

Also, @cbarrete , can you say a little bit about your use case for this?

@cbarrete
Copy link
Contributor Author

I just used --build because that's what Neil suggested first, likely without thinking too much about it. I'm happy to rename it to whatever fits best, change the details of how things get wired, etc. I'll leave the bikeshedding to people who actually have a clear vision for the CLI!

Also, @cbarrete , can you say a little bit about your use case for this?

Ultimately, I'm trying to simplify/improve our CI setup. Right now we have a script that looks vaguely like:

targets=$(supertd stuff)
buck2 build --skip-incompatible-targets $targets
buck2 build --skip-incompatible-targets $targets --target-platforms //non/default:platform
buck2 build --skip-incompatible-targets $targets --target-platforms //some/other:platform

buck2 test --skip-incompatible-targets $targets
buck2 test --skip-incompatible-targets $targets --target-platforms //non/default:platform
buck2 test --skip-incompatible-targets $targets --target-platforms //some/other:platform

Which I'd ideally like to turn into:

targets=$(supertd stuff)
buck2 test $targets --configurations //default:platform //non/default:platform //some/other:platform

I'm omitting test runner business, but I want something that lets me send individual test logs to a backend for a nicer UX than everything in one file. I'll also gloss over the configuration stuff, that's a separate issue (in short, it would be nice to not run them sequentially, but also some targets in different language that aren't affected by the target incompatibility end up being tested multiple times, which is suboptimal).

So what's relevant here is merging the build and test steps together, which is both nicer and more efficient (fewer DAGs = more goodness overall).

The end goal is to make CI so vanilla that what devs run locally is as close to what runs remotely as possible by default. The differences would be the supertd business, which is ok to me, and something people can opt into; and the custom test runner, which is not relevant locally. I have a personal opinion that it would be a GoodThing for CI systems to be very thin and leverage good build systems as much as possible, so that plays into that as well.

@JakobDegen
Copy link
Contributor

Alright, since people internally seem to have surprisingly few opinions on this, I say let's land it without changing the default for now. That being said, I would ask you to switch away from a --build flag and instead offer both --build-run-info and --build-default-info, matching what buck2 build has

@cbarrete
Copy link
Contributor Author

cbarrete commented Jul 9, 2024

Hey, I haven't forgotten about that, but just haven't had the time to finish this up. I hope to get to it before the end of the week though.

@facebook-github-bot
Copy link
Contributor

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

@cbarrete
Copy link
Contributor Author

Hello @JakobDegen, is something blocking this diff internally? Is there something I can do to help move this forward?

@cbarrete
Copy link
Contributor Author

@Nero5023 This PR has been stuck for a few months, do you know if someone/who could look at it?

I have a few more PRs that I'm having trouble getting reviewed: https://github.com/facebook/buck2/pulls/cbarrete

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants