Skip to content

Conversation

@JosiahWI
Copy link
Contributor

This fixes an issue on case-insensitive filesystems where the BUILD item in .gitignore causes the build/ directory to be ignored in git. It also opens up build/ for use as an out of source build directory, which is a very common idiom for CMake users.

@JosiahWI JosiahWI self-assigned this Jun 26, 2023
@JosiahWI JosiahWI added the Build work related to build configuration or environment label Jun 26, 2023
@JosiahWI JosiahWI requested a review from bneradt June 26, 2023 17:02
@bneradt bneradt dismissed their stale review June 26, 2023 17:35

Josiah did the review comment.

@JosiahWI JosiahWI added this to the 10.0.0 milestone Jun 26, 2023
Copy link
Contributor

@bneradt bneradt left a comment

Choose a reason for hiding this comment

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

Looks good. I'll let a non-Yahoo committer give you an approval.

@bryancall bryancall requested a review from cmcfarlen June 26, 2023 22:07
@moonchen
Copy link
Contributor

Please also update README.md to reflect the change in directory structure.

JosiahWI added 4 commits June 27, 2023 06:37
This fixes an issue on case-insensitive filesystems where the
BUILD item in .gitignore causes the build/ directory to be ignored
in git. It also opens up build/ for use as an out of source build
directory, which is a very common idiom for CMake users.
@JosiahWI JosiahWI force-pushed the feat/rename-build branch from d69a73f to 482b6e4 Compare June 27, 2023 11:39
@JosiahWI
Copy link
Contributor Author

Rebased on master.

@JosiahWI
Copy link
Contributor Author

[approve ci autest]

@JosiahWI JosiahWI requested a review from bryancall June 29, 2023 01:24
^BUILDS$
^autom4te\.cache$
^build$
^m4$
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want rat to check our m4 files? I wonder if this line should just be removed? @bneradt do you have any input?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RAT checks our CMake scripts. This seems like an analogous case for automake. Just my two cents.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably good to check them. Let's do that as a separate PR though in case something goes wrong or we change our mind so it would be easy to revert.

@bneradt
Copy link
Contributor

bneradt commented Jun 29, 2023

[approve ci autest]

@jpeach
Copy link
Contributor

jpeach commented Jun 29, 2023

@JosiahWI Can you also remove "BUILD" from the gitignore? It looks like @zwoop added it in #8428, but it looks accidental to me.

It looks like it was accidentally added, so this removes it. I
also put in a comment explaining why build/ is now in the .gitignore.
@bneradt
Copy link
Contributor

bneradt commented Jun 29, 2023

[approve ci]

@cmcfarlen
Copy link
Contributor

[approve ci ubuntu]

@bneradt
Copy link
Contributor

bneradt commented Jun 29, 2023

[approve ci cmake]

Copy link
Contributor

@cmcfarlen cmcfarlen left a comment

Choose a reason for hiding this comment

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

I feel like enough time has passed for someone to object to this. Approved!

@bneradt bneradt merged commit 7156a30 into apache:master Jul 1, 2023
@JosiahWI JosiahWI deleted the feat/rename-build branch July 5, 2023 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Build work related to build configuration or environment Cleanup

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants