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 CMake #46

Merged
merged 8 commits into from
Nov 6, 2018
Merged

Switch to CMake #46

merged 8 commits into from
Nov 6, 2018

Conversation

ZetaTwo
Copy link
Contributor

@ZetaTwo ZetaTwo commented Oct 14, 2018

I looked into supporting building multiple architectures at the same time, for example to be able to have both 32bit and 64bit versions of preeny at the same time. The problem with the old Makefile if that it always used the host's arch for naming the output directory. As I started digging more into the Makefile I couldn't find a sane way to do it and after a while I experimented with using CMake instead.

There are of course pros & cons with this change:
Pros:

  • IMO: much easier to read CMakeList than traditional Makefiles
  • Trivial to build multiple configurations simultaneous

Cons:

  • Adds dependency on CMake

I also added a test runner script for the test binaries which is copied into the build directory to make it super simple to run the tests.

Unfortuntaley, libini-config on Ubuntu can't co-exist with both 32 and 64 bit versions anyway so this still isn't a complete solution to building 32 and 64 bit at the same time.

What do you think?

@zardus
Copy link
Owner

zardus commented Oct 15, 2018

I'm hesitant to take this change wholesale. I'm not necessarily super proficient with cmake, and accepting analogous changes in other projects (an unfamiliar testing framework in ctf-tools) led to that component simply atrophying. I'm not comfortable replacing the Makefiles, imperfect as they are, with cmake.

That being said, I'm totally happy with cmake being a build system alongside make. If you can amend this PR to support that (probably just undeleting the Makefiles), I'm all for it!

@ZetaTwo
Copy link
Contributor Author

ZetaTwo commented Oct 18, 2018

Totally respect that stance. I really wish I was better at make then I would just improve the Makefile. I will update the PR to keep the Makefiles and let this live in parallel. Expect updated PR this weekend.

@zardus
Copy link
Owner

zardus commented Oct 24, 2018

Looks like the cross-compilation Makefile stuff was fixed in #50! If you rebase this to remove the Makefile changes, I think we can go ahead and merge it!

@zardus
Copy link
Owner

zardus commented Nov 2, 2018

Ping :-)

@ZetaTwo
Copy link
Contributor Author

ZetaTwo commented Nov 4, 2018

Sorry for the delay. Now I think this PR is less intrusive and should be mergable.

@zardus zardus merged commit 32d2ce9 into zardus:master Nov 6, 2018
@zardus
Copy link
Owner

zardus commented Nov 6, 2018

Sweet, thanks!

@ZetaTwo ZetaTwo deleted the cmake branch November 6, 2018 10:25
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