Skip to content

Add a placeholder for the test-suite-externals directory #186

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

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

mstorsjo
Copy link
Member

@mstorsjo mstorsjo commented Dec 7, 2024

When setting up external sources within llvm-test-suite, that can either be done by checking out those sources in the test-suite-externals directory, or by setting a CMake variable to point at the source directory elsewhere.

As the git repo itself doesn't contain any files in the test-suite-externals directory, no such directory exists in users checkouts, unless they manually create it (which feels brittle and can be prone to mistypings etc).

Therefore, add a placeholder dummy file to force git to create the directory, making it more obvious to users where they can place external sources. The directory is ignored via .gitignore, so files or directories in this tree don't get added accidentally.

When setting up external sources within llvm-test-suite, that can
either be done by checking out those sources in the
test-suite-externals directory, or by setting a CMake variable to
point at the source directory elsewhere.

As the git repo itself doesn't contain any files in the
test-suite-externals directory, no such directory exists in users
checkouts, unless they manually create it (which feels brittle
and can be prone to mistypings etc).

Therefore, add a placeholder dummy file to force git to create
the directory, making it more obvious to users where they can place
external sources. The directory is ignored via .gitignore, so files
or directories in this tree don't get added accidentally.
Copy link
Contributor

@sjoerdmeijer sjoerdmeijer left a comment

Choose a reason for hiding this comment

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

This makes a lot of sense, I think this is an improvement, so LGTM.

@fhahn
Copy link
Contributor

fhahn commented Dec 9, 2024

Won't this break checkouts for users already having a test-suite-externals directory in their clone?

@mstorsjo
Copy link
Member Author

mstorsjo commented Dec 9, 2024

Won't this break checkouts for users already having a test-suite-externals directory in their clone?

No - this doesn't come with an explicit intent of "create a new directory", it'll just add the .gitkeep file in the existing directory, or create the directory if needed. (Otherwise you'd have the same issue if you switch between branches that have a directory and ones that lack that directory, if you'd happen to place extra files in that directory, e.g. *.o build outputs.)

Copy link
Contributor

@fhahn fhahn left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for clarifying

@mstorsjo mstorsjo merged commit d9dd17a into llvm:main Dec 9, 2024
1 check passed
@mstorsjo mstorsjo deleted the test-suite-externals-gitkeep branch December 9, 2024 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants