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

CMake build system support #17

Merged
merged 7 commits into from
Dec 4, 2017
Merged

Conversation

hoijui
Copy link
Contributor

@hoijui hoijui commented Sep 23, 2017

adds basic support for building and installing CMatrix through CMake (a cross-platform meta-build system).
the binary should compiled on all platforms (untested), but the fonts are only installed on Unix systems.

Copy link
Owner

@abishekvashok abishekvashok left a comment

Choose a reason for hiding this comment

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

Please check the failing tests in Travis as well.

cmatrix.c Outdated
@@ -32,8 +32,6 @@
#include <termios.h>
#include <signal.h>

#include "config.h"
Copy link
Owner

Choose a reason for hiding this comment

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

config.h is generated using make and is needed for some configuration options that we will leave for the user.

@hoijui
Copy link
Contributor Author

hoijui commented Sep 29, 2017

i added a simple fix for config.h

@abishekvashok
Copy link
Owner

@hoijui can you modify the .travis.yml file as well so that we can make sure everything works well there as well.

@hoijui
Copy link
Contributor Author

hoijui commented Oct 8, 2017

ok.. did that, and it seems to work :-)
is it ok that now configure is not tested anymore?
i never used travis before.. did not find out how to do the two of them (i guess that would mean, having two scripts)

@abishekvashok
Copy link
Owner

You can parallise the build. It's important to run tests on configure.

https://docs.travis-ci.com/user/speeding-up-the-build/

If you can't find help. I would help on this.

@hoijui hoijui force-pushed the cmake branch 2 times, most recently from 681484e to 8c8ae0b Compare October 10, 2017 07:14
@hoijui
Copy link
Contributor Author

hoijui commented Oct 10, 2017

thanks for the hint!
looks like now i got it right

.travis.yml Outdated

script:
- ./configure
- mkdir -p build
- cd build
Copy link
Owner

Choose a reason for hiding this comment

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

Cd outside the build and then run configure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so with configure, we can not make an out-of-source build?
why not.. and can we "fix" that?

Copy link
Owner

Choose a reason for hiding this comment

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

Basically the configuration file resides in the root directory of the project as a standard. Since you cd into build, it won't be there in the build folder and you are running ./configure which will make the shell to execute the configure file in ./ i.e, current folder(build).

@hoijui
Copy link
Contributor Author

hoijui commented Oct 13, 2017

ahh...
when you look at the file as the last commit of this bunch leaves it, it looks like this:
https://github.com/hoijui/cmatrix/blob/8c8ae0b4c3b8ebf102e0ec2275e1a0b635591757/.travis.yml
it calls ../configure, and the tests ran fine, and did actually execute both the configure and the cmake way of doing things.

@abishekvashok
Copy link
Owner

Ah my bad, i will test this my local setup and let you know.

@abishekvashok
Copy link
Owner

Yes it works. Thanks for that!

@abishekvashok
Copy link
Owner

@hoijui could you provide a nice commit message so that I could squash and merge or just squash those commits

@hoijui
Copy link
Contributor Author

hoijui commented Oct 19, 2017

... ok! i "beautified" the commit history now, leaving only two commits, which i though make sense separately. i checked that the content is still the same with git diff.

(old history is now under https://github.com/hoijui/cmatrix/tree/cmake-develop)

@hoijui
Copy link
Contributor Author

hoijui commented Oct 19, 2017

ouhh.. now thinking about it, should i also add something to README.md maybe?

This allows for easy copy&paste of build instructions,
and also generates nice output with pandoc now.
You may test that with:

    pandoc -f markdown -t HTML README.md > README.html
use like this:

	mkdir -p build
	cd build
	cmake ..
	# or
    	#cmake -DCMAKE_INSTALL_PREFIX=/usr ..
	make
@hoijui
Copy link
Contributor Author

hoijui commented Oct 19, 2017

i did that, adding 4 READE modifying commits, of which the first 3 are independent of cmake, but for now i included them in this branch anyway. please say if i should separate them out into an other branch/pull request. they are minor though.
also say if i should squash the cmake related README commit into the main cmake commit.

@abishekvashok
Copy link
Owner

Please separate them out. They should go on another issue with another pr. Please, revert those commits, they are causing build errors as well.

@hoijui
Copy link
Contributor Author

hoijui commented Oct 20, 2017

actually, they are not producing the build errors, but the instability of the config build method is. i noted this a few times already, with this very same error (aclocal version mismatch). it seems to pop up randomly, and the way to "fix" it, is to just clean and rebuild.

@hoijui
Copy link
Contributor Author

hoijui commented Oct 20, 2017

it would be easiest if the README related pull request would be merge in first, and then these cmake changes afterwards.

@abishekvashok
Copy link
Owner

@hoijui done.

@hoijui
Copy link
Contributor Author

hoijui commented Dec 4, 2017

thanks for the management work. :-)
from my point of view, this branch could be merged now.

@abishekvashok
Copy link
Owner

@hoijui would be happy if you could provide a nice descriptiuon as the commits will be squashed.

@hoijui
Copy link
Contributor Author

hoijui commented Dec 4, 2017

you mean, you want a commit message for the merge commit, or you do not want to merge, but have a single, rebased, squashed commit?
if the second: why?
because i think, code history should be preserved and be as detailed and divided as possible. when it comes in as a merge of an appropriately named branch, i see no disadvantage for that approach, but many benefits.

a description can be taken from this commit:
9fffb25

@abishekvashok
Copy link
Owner

I think you are right.

@abishekvashok abishekvashok merged commit d4d8079 into abishekvashok:master Dec 4, 2017
@hoijui
Copy link
Contributor Author

hoijui commented Dec 4, 2017

:o wow. cool.. thanks!
really great doing "business" with you! :-)

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