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 main cmake/CMakeLists.txt to CMakeLists.txt #9603

Merged
merged 3 commits into from
Mar 9, 2022

Conversation

Arfrever
Copy link
Contributor

@Arfrever Arfrever commented Mar 8, 2022

cmake/CMakeLists.txt is kept for compatibility and it includes CMakeLists.txt.

Fixes: #9596

@@ -0,0 +1,351 @@
# Minimum CMake required
Copy link
Member

Choose a reason for hiding this comment

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

Could you do a git mv from the old location so that we can preserve the history of this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually did git mv cmake/CMakeLists.txt CMakeLists.txt and later separate git add cmake/CMakeLists.txt for new file.
The only way to do what you want seems to be to have 2 separate commits.
Should I do it?

Copy link
Member

Choose a reason for hiding this comment

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

Actually never mind what I said. From reading some more I think I misunderstood how Git handles file moves and copies. If I understand correctly now, Git doesn't try to express file moves through metadata but instead just tries to detect them after the fact. It's too bad the GitHub UI doesn't show the diff better, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have split those changes into 2 commits, and added a clean-up commit (protobuf_source_dir -> protobuf_SOURCE_DIR), so if you merge these changes as 3 separate commits (not squashed), then history should be better shown.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome! I will take a closer look soon, but my only major concern so far is I think this change is a bit too big to try to get into the 3.20.0 release given that the release is already in progress. Would you mind rebasing this pull request to be against the master branch instead? I can first merge 3.20.x into master if that will help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Please merge 3.20.x into master, and I will re-base these changes against master...

Copy link
Member

Choose a reason for hiding this comment

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

Ok, 3.20.x has now been merged into master.

@Arfrever Arfrever changed the base branch from 3.20.x to master March 9, 2022 03:05
@acozzette acozzette merged commit 4268662 into protocolbuffers:master Mar 9, 2022
@acozzette
Copy link
Member

Thanks, @Arfrever!

@Arfrever Arfrever deleted the cmake branch March 10, 2022 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CMakeLists.txt should be in top-level directory
3 participants