search

ament / uncrustify_vendor

CMake shim over the uncrustify library: https://github.com/uncrustify/uncrustify
Apache License 2.0
0 stars 9 forks source link

Switch to applying patch with git apply. #15

Closed clalancette closed 3 years ago

clalancette commented 3 years ago

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

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

clalancette commented 3 years ago

CI:

cottsay commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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.