Skip to content

Remove git reset and clean step on patch command#3237

Open
hwangsihu wants to merge 1 commit into
masterfrom
git-apply
Open

Remove git reset and clean step on patch command#3237
hwangsihu wants to merge 1 commit into
masterfrom
git-apply

Conversation

@hwangsihu

Copy link
Copy Markdown
Contributor

@CoBC, the Visual Studio files generated by CMake are not suitable for rebuilding. If you want to rebuild, it would be better to delete the relevant files from the cache.

@hwangsihu hwangsihu marked this pull request as ready for review April 22, 2026 12:23
@hwangsihu hwangsihu requested review from CoBC and bear101 as code owners April 22, 2026 12:24
Comment thread Client/Tolk/CMakeLists.txt Outdated
UPDATE_COMMAND ""
PATCH_COMMAND git clean -fdx
COMMAND git apply ${CMAKE_CURRENT_LIST_DIR}/0001-Add-CMake-script-for-building-tolk.patch
PATCH_COMMAND git apply ${CMAKE_CURRENT_LIST_DIR}/0001-Add-CMake-script-for-building-tolk.patch

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this work with MSBuild and Rebuild?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As mentioned earlier, project files generated by CMake are not suitable for rebuilding. To perform a rebuild, it is recommended to delete the relevant files manually.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CMake automatically creates a "clean" target which is used for rebuilding. Both Qt Creator and VS2022 use this target when you choose "Rebuild".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is true that the clean target exists. However, due to the nature of External Projects, using the clean target is not appropriate. External Projects store the completion status of each task as a stamp, but the clean target deletes that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

And now, tolk is no longer used. Since this problem mostly occurred only with tolk, it seems there will be no issues now.

@hwangsihu hwangsihu force-pushed the git-apply branch 3 times, most recently from 60a3c06 to 189a364 Compare May 4, 2026 07:14
@CoBC

CoBC commented May 18, 2026

Copy link
Copy Markdown
Contributor

I disagree with this PR. It breaks build on Windows again. More, I suggest to add reset and clean step on Prism, currently we can't rebuild after first Prism build.

@hwangsihu

Copy link
Copy Markdown
Contributor Author

Adding git reset --hard or git clean -fdx to PATCH_COMMAND would make the patch step idempotent in that specific case, but it does not address the underlying mismatch: the clean target deletes ExternalProject's bookkeeping (stamps in STAMP_DIR), which is not how ExternalProject is designed to be reset. The CMake documentation for PATCH_COMMAND itself notes that defining a robust patch command is "quite difficult" for the same reason.

For Prism, if a clean rebuild is needed, the recommended approach is to delete its build directory manually rather than relying on the clean target.

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