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

Some build system tweaks #1879

Open
wants to merge 37 commits into
base: master
Choose a base branch
from
Open

Conversation

BobSmun
Copy link
Contributor

@BobSmun BobSmun commented Aug 29, 2024

While setting up a dev environment using WSL2 and docker, to better isolate the tools required from those required by other projects, I made the following tweaks.
The primary aim was to improve IDE integration for the debugging process, by allowing the resulting binaries to be run directly from their build output location. Although cmake is pretty resilient in terms of tracking where it places generated outputs, for use in the later install step, I don't have any of the other environments set up (such as xcode or github workflows) to verify that I didn't break any assumptions they may have.

  • Adds .devcontainer to the .gitignore - I think this directory may be vscode + remote container extension specific, so ignoring it in the same vein as ignoring e.g. .vscode
  • Adds .cache and compile_commands.json to the .gitignore, as these are generated files / directories used by clangd (via vscode extension)
  • Enables CMAKE_EXPORT_COMPILE_COMMANDS - useful for other tooling (such as clangd), but harmless on its own
  • Fixed some instances of RUNTIME_OUTPUT_DIRECTORY that should have been LIBRARY_OUTPUT_DIRECTORY
  • For non-xcode builds, altered IMHEX_MAIN_OUTPUT_DIRECTORY to be ${CMAKE_BINARY_DIR}/bin, to separate build outputs from intermediate outputs. Not familiar with xcode, but it appears to already have had some form of separation
  • Updated the build output location, of various components, to be relative to IMHEX_MAIN_OUTPUT_DIRECTORY - this co-locates the build outputs and allows the imhex binary to be run (and debugged) from that build location
  • For non-xcode builds, at cmake configure, either symbolic link directories from the ImHex-Patterns repo clone (if IMHEX_OFFLINE_BUILD) or copy from the download of the patterns (if not IMHEX_OFFLINE_BUILD). This allows the patterns to be available to the build directory instance of imhex, and have any changes between them and the repo be mirrored (if applicable). Again, xcode appears to already have some form of 'copy to build directory' that isn't an install step

@@ -387,7 +387,7 @@ function(configureProject)
# Support Xcode's multi configuration paradigm by placing built artifacts into separate directories
set(IMHEX_MAIN_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/Configs/$<CONFIG>" PARENT_SCOPE)
else()
set(IMHEX_MAIN_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}" PARENT_SCOPE)
set(IMHEX_MAIN_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/bin" PARENT_SCOPE)
Copy link
Owner

Choose a reason for hiding this comment

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

This should probably only be done on Linux as Windows and macOS use a different structure

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 thought that Windows was set up to use MinGW (effectively GCC), which would also work with these changes. In terms of file layout (e.g. executable next to shared objects next to pattern files), I anticipated that they should be compatible though I didn't test the windows build yet.

I also didn't touch the XCODE output path, which I assumed was the macOS build method. I don't have a mac to test with, but it looked like it would correctly separate out the differently configured builds.

else()
# If it is an offline build, the imhex_patterns_SOURCE_DIR location is likely a git clone
# Create the directories as symbolic links, so that any changes get mirrored both ways
file(CREATE_LINK "${imhex_patterns_SOURCE_DIR}/${FOLDER}" "${IMHEX_MAIN_OUTPUT_DIRECTORY}/${FOLDER}" SYMBOLIC)
Copy link
Owner

Choose a reason for hiding this comment

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

Is a symbolic link a good idea? Not all file systems support it.
I don't think the two-way mirroring is really necessary so a simple copy should suffice I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I have been making modifications in the PatternLibrary too, I found it useful to edit in-place next to the binary. I mostly was intending to avoid changes there getting lost.

I notice that there is a COPY_ON_ERROR flag for CREATE_LINK that I seem to have missed, which should fall back to a copy if the symbolic link is not supported.

@WerWolv
Copy link
Owner

WerWolv commented Sep 15, 2024

Thanks a lot!

@BobSmun
Copy link
Contributor Author

BobSmun commented Sep 21, 2024

Tested these changes under windows (using msys2)

  • Re-added RUNTIME_OUTPUT_DIRECTORY where I replaced it with LIBRARY_OUTPUT_DIRECTORY (so both are now present), as the windows build seems to use that for shared libraries
  • libpl had a specific override of the various OUTPUT_DIRECTORYs, only for windows, that was missed

@BobSmun BobSmun marked this pull request as draft December 1, 2024 09:40
* Reorganize plugin tests, to fix test registration
* Link manually cloned ImHex-Patterns to cmake (fixes code completion and adds unit tests)
* Fix IMHEX_STATIC_LINK_PLUGINS to at least build
… to not complain about missing CMakeLists of currently not merged ImHex-Patterns
@BobSmun
Copy link
Contributor Author

BobSmun commented Dec 11, 2024

Whole bunch of work to fix macOS
Also fixed up unit tests (including previously disabled and broken plugin tests)
Added windows runner for unit tests, as there was one point where linux unit tests were passing when windows ones were failing
Link manually cloned ImHex-Patterns into cmake

@BobSmun BobSmun marked this pull request as ready for review December 11, 2024 16:42
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.

2 participants