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

A new binary object which tests linking with libfontforge.la #553

Merged
merged 5 commits into from
May 31, 2013

Conversation

coolwanglu
Copy link
Contributor

This is a first step towards #465 and #469.

I added a new near-empty c file, which links to libfontforge.la. The file is in the tests folder, which will not be installed.

This target is a normal target in tests, which is always compiled, not only when make check is called.(Sorry about the commit message) I made this because actually I don't know how to make it that way. Or shall I write a new script in the testsuite? That way it would be harder to handle compilers, flags, and paths -- but possible.

Since it is a normal target, if this pull request is merged, make will fail, so #465 should be fixed beforehand.

Also please check if any FLAGS should be added for this target.

BTW, why there are xxx_CPPFLAGS used in many makefiles? I don't think we have CPP files.

@ghost
Copy link

ghost commented Apr 19, 2013

CPPFLAGS sets flags for the C preprocessor. Flags for a C++ compiler (which compiles .cpp files) would be CXXFLAGS. I think this is for historical reasons - CPPFLAGS existed before C++.

I'm not sure that this test should properly be done as part of "make check," because it's not really a test of our code but of the build process itself. I think having it fail during plain "make" may be the right thing; and that's certainly easier to accomplish, just tell Automake to build but not install the binary that results from link_test.c. Breaking libfontforge is a big problem, and I think it should break the build. Detecting it only when "make check" is invoked means people will continue committing changes that cause this problem, because they won't bother running "make check" before committing.

If you did want to make it strictly part of "make check", however, I think the right way to do it instead of trying to write a test script that understands compilers and paths and so on, would be to have a target in the Makefile for building the binary that results from link_test.c, but not tell Automake to build that binary as part of "make all". That's easy to accomplish by leaving it out of the relevant _PROGRAMS variables. Then have a test script that just invokes "make link_test". That way Autotools can worry about getting the flags right. It won't be completely bulletproof (may fail, in particular, with VPATH builds or on systems where make is not named "make"), but such things don't need to be perfect.

@coolwanglu
Copy link
Contributor Author

@mskala Ah yes, when I see the CXXFLAGS you mentioned, I just remember the tricky gotcha "CC is for C compilers, but not C++ as in .cc files"...

@davelab6
Copy link
Member

Can this now be merged? :)

@coolwanglu
Copy link
Contributor Author

@davelab6 It can be 'merged', but then make would fail, since the new dummy target cannot be linked.

@davelab6
Copy link
Member

Ahh I see. Okay. Let's wait for the issues to be fixed now and then merge this so there is no regression on this :)

@coolwanglu
Copy link
Contributor Author

Seems that we are now clear to merge this one.

@coolwanglu
Copy link
Contributor Author

@davelab6 I think we are ready for this merge. Shall I do so?

@davelab6
Copy link
Member

Sure! No need to wait for my approval:-)
On May 30, 2013 11:30 PM, "Lu Wang" notifications@github.com wrote:

@davelab6 https://github.com/davelab6 I think we are ready for this
merge. Shall I do so?


Reply to this email directly or view it on GitHubhttps://github.com//pull/553#issuecomment-18721606
.

coolwanglu added a commit that referenced this pull request May 31, 2013
A new binary object which tests linking with libfontforge.la
@coolwanglu coolwanglu merged commit 0e09da0 into fontforge:master May 31, 2013
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.

3 participants