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

Switch to applying patch with git apply. #15

Merged
merged 1 commit into from
Dec 23, 2020

Conversation

clalancette
Copy link
Contributor

This gets us closer to being able to build without Administrator
on Windows, since the "patch" command required Administrator.

Signed-off-by: Chris Lalancette [email protected]

This gets us closer to being able to build without Administrator
on Windows, since the "patch" command required Administrator.

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette clalancette requested a review from hidmic December 16, 2020 15:58
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM pending green CI

@clalancette
Copy link
Contributor Author

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@clalancette clalancette merged commit ec709bb into master Dec 23, 2020
@delete-merged-branch delete-merged-branch bot deleted the clalancette/switch-to-git-apply branch December 23, 2020 15:38
@cottsay
Copy link
Contributor

cottsay commented Jan 5, 2021

@clalancette, this is almost certainly what is breaking the Windows builds on ci.ros2.org. Your CI jobs above are configured to only run tests for this package, and there are no tests for this package (which is a problem as well).

It's interesting that the patch command succeeds but didn't work.

@clalancette
Copy link
Contributor Author

@clalancette, this is almost certainly what is breaking the Windows builds on ci.ros2.org. Your CI jobs above are configured to only run tests for this package, and there are no tests for this package (which is a problem as well).

Bah, humbug. Sorry about that.

If you can open a revert PR, that would be great. If not, I'll open one tomorrow morning. Thanks for tracking this down.

@cottsay
Copy link
Contributor

cottsay commented Jan 6, 2021

Alright, I got to the bottom of why this is failing. It appears that git-apply CAN work outside of a git tree, when invoked in a git tree, the paths are always considered relative to the root of the repo. So if any parent directories are a repo, the command will fail. Worse yet, it seems to silently skip the patches for some reason.

So the easiest way to make this work is just to make the root a git repo. This worked for me:

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -40,11 +40,10 @@ macro(build_uncrustify)

   # Pinning to tip of master at the time of a desired bugfix
   set(uncrustify_version 45b836cff040594994d364ad6fd3f04adc890a26)
-  set(uncrustify_version_hash 7cc32ad800c8d639bdf145e2a113a66b)
   # Get uncrustify 0.68.1
   ExternalProject_Add(uncrustify-0.68.1
-    URL https://github.com/uncrustify/uncrustify/archive/${uncrustify_version}.tar.gz
-    URL_MD5 ${uncrustify_version_hash}
+    GIT_REPOSITORY https://github.com/uncrustify/uncrustify.git
+    GIT_TAG ${uncrustify_version}
     TIMEOUT 600
     CMAKE_ARGS
       -DCMAKE_INSTALL_PREFIX=${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}_install

Should I make a PR with the change to use git to fetch the sources, or make a PR to revert this one until we find a better solution?

@clalancette
Copy link
Contributor Author

Should I make a PR with the change to use git to fetch the sources,

I like your proposed change. It keeps the property I was going for with this PR (build on Windows without Administrator), and it fixes the regression I introduced. Please open a PR with that change, and I'm happy to review it.

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