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.

Sign up for GitHub

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

Jump to bottom

Use CMake FetchContent to support both stable and HEAD ghidra #15

Merged
merged 8 commits into from
Nov 17, 2021
Merged

Use CMake FetchContent to support both stable and HEAD ghidra #15

merged 8 commits into from
Nov 17, 2021

Conversation

ekilmer
Copy link
Contributor

@ekilmer ekilmer commented Nov 12, 2021 •
edited
Loading

closes #14

TODO:

  • More testing to make sure this method plays nice with different workflows of people (especially the FETCHCONTENT_BASE_DIR)
  • Updating the README to make sure people know what's going on and/or how to use their own Ghidra source checkout.

Would appreciate people testing it out and giving comments!

Another thing that would be nice to have eventually is weekly testing of new HEAD commits from Ghidra main repo. To be fully automated, I think we would have to do something similar to dependabot but update the src/setup-ghidra-source.cmake line with

set(ghidra_git_tag "<head commit>")

Would also be nice to auto-generate the slaspec file listings.

@ekilmer
Don't put FetchContent logic in CMakeLists.txt file
c5a22eb 
Added README directions if you want to get fancy. This also reduces
maintenance and support effort.
@ekilmer ekilmer marked this pull request as ready for review November 15, 2021 21:17
@ekilmer ekilmer mentioned this pull request Nov 15, 2021
Merged
@tetsuo-cpp
Copy link
Contributor

tetsuo-cpp commented Nov 16, 2021 •
edited
Loading

Thanks, this is awesome. I had a problem switching to HEAD after I'd built stable.

tetsuo@Alexs-MacBook-Pro sleigh % cmake -B build -S . -DSLEIGH_ENABLE_INSTALL=ON -DSLEIGH_GHIDRA_RELEASE_TYPE=HEAD
-- Using Ghidra version 10.0.4 at commit 55b8fcf
-- Populating ghidrasource
-- Configuring done
-- Generating done
-- Build files have been written to: /Users/tetsuo/Code/sleigh/build/_deps/ghidrasource-subbuild
[0/7] Performing update step for 'ghidrasource-populate'
[2/7] Performing patch step for 'ghidrasource-populate'
FAILED: ghidrasource-populate-prefix/src/ghidrasource-populate-stamp/ghidrasource-populate-patch /Users/tetsuo/Code/sleigh/build/_deps/ghidrasource-subbuild/ghidrasource-populate-prefix/src/ghidrasource-populate-stamp/ghidrasource-populate-patch
cd /Users/tetsuo/Code/sleigh/build/_deps/ghidrasource-src && git am --ignore-space-change --ignore-whitespace --no-gpg-sign /Users/tetsuo/Code/sleigh/patches/HEAD/0001-Fix-arg-parsing-in-sleigh-C-test-runner.patch && /opt/homebrew/Cellar/cmake/3.21.3/bin/cmake -E touch /Users/tetsuo/Code/sleigh/build/_deps/ghidrasource-subbuild/ghidrasource-populate-prefix/src/ghidrasource-populate-stamp/ghidrasource-populate-patch
fatal: previous rebase directory .git/rebase-apply still exists but mbox given.
ninja: build stopped: subcommand failed.
CMake Error at /opt/homebrew/Cellar/cmake/3.21.3/share/cmake/Modules/FetchContent.cmake:1017 (message):
  Build step for ghidrasource failed: 1
Call Stack (most recent call first):
  /opt/homebrew/Cellar/cmake/3.21.3/share/cmake/Modules/FetchContent.cmake:1146:EVAL:2 (__FetchContent_directPopulate)
  /opt/homebrew/Cellar/cmake/3.21.3/share/cmake/Modules/FetchContent.cmake:1146 (cmake_language)
  /opt/homebrew/Cellar/cmake/3.21.3/share/cmake/Modules/FetchContent.cmake:1189 (FetchContent_Populate)
  src/setup-ghidra-source.cmake:54 (FetchContent_MakeAvailable)
  CMakeLists.txt:13 (include)


-- Configuring incomplete, errors occurred!
See also "/Users/tetsuo/Code/sleigh/build/CMakeFiles/CMakeOutput.log".
See also "/Users/tetsuo/Code/sleigh/build/CMakeFiles/CMakeError.log".

It seems to be trying to apply the patch even though the Ghidra source hasn't been checked out to the HEAD commit, and therefore failing. I had a quick look at the FetchContent docs but didn't see anything obvious.

I'll try to debug it tomorrow unless you get around to it by then.

tetsuo-cpp
tetsuo-cpp reviewed Nov 16, 2021
View reviewed changes
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/setup-ghidra-source.cmake Show resolved Hide resolved
@ekilmer
Copy link
Contributor Author

ekilmer commented Nov 16, 2021 •
edited
Loading

I had a problem switching to HEAD after I'd built stable.

Ahh, yeah this is a known issue due to the way I implemented the logic. Basically, I used more cache variables for specifying the commit SHA and numeric Ghidra version number, which would also need to be changed accordingly upon re-configuration from a stable release to a HEAD release. If you try the following sequence, it should work.

$ cmake -B build -S . -DSLEIGH_ENABLE_INSTALL=ON
...
$ cmake --build build -j
...
$ cmake -B build -S . -DSLEIGH_ENABLE_INSTALL=ON \
    -DSLEIGH_GHIDRA_RELEASE_TYPE=HEAD \
    -DSLEIGH_GHIDRA_VERSION=10.1 \
    -DSLEIGH_GHIDRA_COMMIT=55b8fcf7d4aeaca37ffb5c947340915d69c84224
...
$ cmake --build build -j
...

I think the workflow might be a matter of preference, but I'm happy to discuss. I see two options for resolving this:

  1. Tell people in the README that they should use separate build directories for stable and HEAD builds; or if they want to reconfigure the same build directory, then they must specify the other details as they appear in the setup script.
  2. Remove the other cache variables leaving only the SLEIGH_GHIDRA_RELEASE_TYPE cache variable. This reduces the ability to customize without changing the source, but it also makes our lives easier by not supporting a workflow that involves some somewhat hidden details, as you've found out.

I'm actually leaning towards option 2 because it reduces our pain of dependent cache variables and makes the configuration a bit more streamlined. If an end user wants to use their own arbitrary checkouts, then they should use the method supported by FetchContent to point to their own source directory. That method also makes it more obvious that what they're doing likely isn't officially supported. Furthermore, updating to a different HEAD version requires a change to the source code, which could be better from a visibility standpoint when testing upstream changes.

Thoughts?

Edit:
Option 2 is implemented in a3c7ca2

You should now be able to reconfigure as you did in your original comment.

cmake -B build -S . -DSLEIGH_ENABLE_INSTALL=ON
...
cmake --build build -j
...
cmake -B build -S . -DSLEIGH_ENABLE_INSTALL=ON -DSLEIGH_GHIDRA_RELEASE_TYPE=HEAD
...
cmake --build build -j

@ekilmer
More documentation on the Dependencies and Prerequisites in README
23a021f
@ekilmer
Use ON instead of TRUE for consistency
e7b29e7
@ekilmer
Use fewer cache variables during Ghidra source setup
a3c7ca2 
This should make it easier to switch between stable and HEAD commits
without having to specify other somewhat hidden cache variables
This was referenced Nov 16, 2021
Merged
Closed
tetsuo-cpp
tetsuo-cpp approved these changes Nov 17, 2021
View reviewed changes
Copy link
Contributor

@tetsuo-cpp tetsuo-cpp left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks, this works great. Feel free to merge whenever you're ready.

@ekilmer ekilmer merged commit 3e1e6a7 into master Nov 17, 2021
@ekilmer ekilmer deleted the support-stable-plus-head branch November 17, 2021 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tetsuo-cpp tetsuo-cpp tetsuo-cpp approved these changes

@alessandrogario alessandrogario Awaiting requested review from alessandrogario

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support both stable and latest (pinned commit) Ghidra
2 participants
@ekilmer @tetsuo-cpp