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

docs: there may be multiple BUILD files in a directory #20818

Conversation

AlexTereshenkov
Copy link
Member

@AlexTereshenkov AlexTereshenkov commented Apr 19, 2024

Docs rendered locally:

image

may want to use as part of your builds. However, you can have multiple BUILD files in a single directory.
When running a Pants goal, the contents of the BUILD files will be merged making it possible to better group your
targets, if desired. For instance, you could store source code targets and
[macros](../../writing-plugins/macros.mdx) in different files to have better separation of concerns:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link tested.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For instance, you could store source code targets and
macros in different files to have better separation of concerns

The example seems to show having different targets in the files, which works fine, but the above text talks about macros, and that will not work. The files are not merged before parsing, so if you decalare a def my_macro(): ... in one, you can't call it from the other.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is, they're parsed, and processed, individually, they will just share "namespace" for their respective targets, so they show up together..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Andreas! Perhaps bringing macros into this wasn't worth it. I've updated the docs to use files and code targets; this should keep things simple.

@AlexTereshenkov AlexTereshenkov marked this pull request as ready for review April 19, 2024 09:28
@AlexTereshenkov AlexTereshenkov force-pushed the docs/multiple-build-files-per-directory branch from a1d61a4 to 00daf2b Compare April 19, 2024 20:05
@huonw
Copy link
Contributor

huonw commented Apr 20, 2024

Thanks for improving the documentation! I think that we should be careful with how we phrase this: I think that for most users, having everything in one BUILD file works best, and trying to split an individual file up is likely to just cause confusion. It seems reasonable to describe that it is possible, though.

A couple of questions:

  1. Do you use this in practice?
  2. The BUILD.experimental example sounds good; maybe we could use that as the only example?
  3. Maybe we could also add an info/warning box that explicitly suggests that "one BUILD is usually what you want", too?

@kaos
Copy link
Member

kaos commented Apr 22, 2024

Thanks for improving the documentation! I think that we should be careful with how we phrase this: I think that for most users, having everything in one BUILD file works best, and trying to split an individual file up is likely to just cause confusion. It seems reasonable to describe that it is possible, though.

I agree with ☝🏽

A couple of questions:

  1. Do you use this in practice?

See this thread: https://chat.pantsbuild.org/t/16949338/is-there-a-way-to-exclude-an-arbitrary-build-target-at-runti#45196675-a2bb-4472-a837-e4de911f6898

  1. The BUILD.experimental example sounds good; maybe we could use that as the only example?
  2. Maybe we could also add an info/warning box that explicitly suggests that "one BUILD is usually what you want", too?

@cognifloyd
Copy link
Member

cognifloyd commented Jun 19, 2024

  1. Do you use this in practice?

I use this in the StackStorm repo to separate tool resolve requirements from the primary code requirements targets:
https://github.com/StackStorm/st2/blob/master/BUILD.tools
https://github.com/StackStorm/st2/blob/master/BUILD

@AlexTereshenkov AlexTereshenkov force-pushed the docs/multiple-build-files-per-directory branch from 00daf2b to 1aa56e3 Compare June 20, 2024 21:12
@AlexTereshenkov
Copy link
Member Author

Thanks for improving the documentation! I think that we should be careful with how we phrase this: I think that for most users, having everything in one BUILD file works best, and trying to split an individual file up is likely to just cause confusion. It seems reasonable to describe that it is possible, though.

A couple of questions:

1. Do you use this in practice?

2. The `BUILD.experimental` example sounds good; maybe we could use that as the only example?

3. Maybe we could also add an info/warning box that explicitly suggests that "one `BUILD` is usually what you want", too?

I am sorry for the late reply.

1. Do you use this in practice?

Yes, I do. We have some targets that I only want to become available at CI builds as there is some code generation done before Pants runs.

2. The `BUILD.experimental` example sounds good; maybe we could use that as the only example?

Definitely, I've updated the docs.

3. Maybe we could also add an info/warning box that explicitly suggests that "one `BUILD` is usually what you want", too?

I've added a sentence at the beginning of the paragraph to make it clearer.

@AlexTereshenkov AlexTereshenkov added the release-notes:not-required PR doesn't require mention in release notes label Jun 20, 2024
@AlexTereshenkov AlexTereshenkov merged commit ad47fe4 into pantsbuild:main Jun 20, 2024
25 checks passed
@AlexTereshenkov AlexTereshenkov deleted the docs/multiple-build-files-per-directory branch June 20, 2024 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:documentation release-notes:not-required PR doesn't require mention in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants