-
Notifications
You must be signed in to change notification settings - Fork 281
Add testsuite as a submodule of c2rust #1473
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
base: master
Are you sure you want to change the base?
Conversation
kkysen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this trying to add the testsuite to c2rust directly? If so, why not just inline it directly? That seems a lot simpler instead of having multiple levels of submodules. Putting them in the same repo also gives the important benefit of being able to make changes across both at once, which a submodule doesn't.
We could inline it directly using |
|
By "simpler" I mean it doesn't duplicate an entire repository (with its history and contents) in a second place. |
|
Now we have $ROOT/tests and $ROOT/testsuite which is a bit messy. Doesn't have to be this PR but how about moving stuff under |
What benefit does the submodule give us, though? I'm just not sure what the motivation is. Inlining it (not as a subtree, although I don't know much about git subtrees) gives us the major benefit of PRs and commits that can span both atomically. |
| path: testsuite | ||
| submodules: true | ||
| # Import all submodules including the test suite and its sub-submodules | ||
| submodules: recursive |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are submodules inside the c2rust-testsuite's submodules that we don't want to clone. If I'm remembering right when I tried git clone --recursive.
All updates to the testsuite would have to go through the main repository (you have to update the submodule explicitly through a top-level commit), which would avoid today's problems. That would be nice to have, but not critical. |
Does that mean for every change in the testsuite repo, we then have to make a separate PR to update the submodule to the new master? That seems like a lot of extra work, if I'm understanding how it works correctly. |
This imports and runs the test suite through the main repository, mapping every commit in c2rust to an exact commit in c2rust-testsuite. Any updates to the suite have to go through the main repository, which means the suite cannot be updated underneath us anymore.