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

Rename the root cell #687

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

Rename the root cell #687

wants to merge 1 commit into from

Conversation

cbarrete
Copy link
Contributor

This commit renames the root cell to facilitate embedding it as a dependency to Rust projects. While the Buck2 repository does not have an official public API, depending on it is the most reasonable way of writing custom test runners, which is important for OSS users until TestInfo V2 lands.

root is a common name for root cells, it is generic and it is the default when running buck2 init, making it a bad name that is not descriptive and likely to conflict with downstream cells.

As described in the added docs, this commit is not an ideal solution, since it requires configuration hacks:

  • The root .buckconfig must point to Buck2's shim directory.
  • Buck2's .buckconfig must be edited to remove cells that are already defined in the root.

Relates to #684.

@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 18, 2024
@cbarrete
Copy link
Contributor Author

Hello @JakobDegen, do you know who I should tag to have a look at this? I'm very open to feedback, especially on the caveats that I outlined. I'd like to remove them, but I don't know what would be a good way.

@zjturner
Copy link
Contributor

What about places like this that use an implicit root?

https://github.com/facebook/buck2/blob/main/app/buck2_test_runner/BUCK#L18-L21

@cbarrete
Copy link
Contributor Author

I also thought they would have to be made explicit, but as I understand it, a bare // means "the current cell" rather than "the root cell". If I'm not mistaken, there are no further changes required, which is nice.
Since the build/CI pass just fine, and I don't have root cell conflicts anymore with my experiment, I'm fairly confident that this is correct.

@cbarrete
Copy link
Contributor Author

I'm not sure who else to tag, could you point me to the right person @ndmitchell?
I'm also happy to update CONTRIBUTING.md with how it is appropriate to ask for review of pull requests if you let me know what is preferred.

@JakobDegen
Copy link
Contributor

Sorry this took so long. This looks fine to me in principle, I'll see what I need to do to get it through CI internally too

@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.

@JakobDegen
Copy link
Contributor

Oh, oof, could you rebase this? Otherwise should be easy to get merged

This commit renames the `root` cell to facilitate embedding it as a dependency to Rust projects.
While the Buck2 repository does not have an official public API, depending on it is the most
reasonable way of writing custom test runners, which is important for OSS users until TestInfo V2
lands.

`root` is a common name for root cells, it is generic and it is the default when running `buck2
init`, making it a bad name that is not descriptive and likely to conflict with downstream cells.

As described in the added docs, this commit is not an ideal solution, since it requires
configuration hacks:
- The root `.buckconfig` must point to Buck2's shim directory.
- Buck2's `.buckconfig` must be edited to remove cells that are already defined in the root.
@cbarrete
Copy link
Contributor Author

@JakobDegen Is something blocking this PR?

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.

4 participants