Skip to content

configure: optional build documentation #212

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

Merged
merged 6 commits into from
Sep 8, 2019

Conversation

mikkoi
Copy link
Contributor

@mikkoi mikkoi commented Aug 19, 2019

Fix issue #206

Applied an uncommitted patch by Branden Archer.
https://sourceforge.net/p/check/bugs/101/

Signed-off-by: Mikko Johannes Koivunalho mikko.koivunalho@iki.fi

@mikkoi mikkoi force-pushed the is-makeinfo-required branch 2 times, most recently from 49e2c49 to 4ad4f87 Compare August 19, 2019 21:46
lib/snprintf.c Outdated
@@ -1097,6 +1097,9 @@ fmtflt(char *str, size_t *len, size_t size, LDOUBLE fvalue, int width,
infnan = (flags & PRINT_F_UP) ? "INF" : "inf";

if (infnan != NULL) {
int i = MAX_CONVERT_LENGTH - 1;
while (i >= 0) /* Initialize buffer, do not use memset (compatility) */
iconvert[i--] = '\0';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use memset?

Also, why not do this at the beginning at line 1084?

Should the same be done for fconvert, at least for consistency?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also (not a requirement, just a request), could you send the patch back to the original author as well?

http://www.jhweiss.de/software/snprintf.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Function memset() is not used anywhere in libcompat. So I figured maybe someone had already come to the conclusion that memset() is not available at every C installation or other compatibility issue. A little backwards logic here, I know. Just being overly careful.

However, memset() is used in libcheck in three places. If you say that it is okay to use memset(), I'm all for it.

I will move it to the beginning of the function, line 1084. A good idea, since the buffer is also used later in the function.

Same for fconvert? I will change that too.

* We need the GNU C version to prevent using pragmas where they are
* not supported.
*/
#define GCC_VERSION (__GNUC__ * 10000 \
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be in libcompat instead? Or, is one goal to reduce/remove that header eventually?

lib/libcompat.h Outdated
@@ -45,6 +45,11 @@
#define CK_ATTRIBUTE_NORETURN
#endif /* GCC 2.5 */

/* Actually GCC 4.6.4 */
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, you did move it to the header.

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 mean to improve the GCC_VERSION_AT_LEAST(major, minor) macro to GCC_VERSION_AT_LEAST(major, minor, patch) because the patch number is useful in some circumstances, such as here.

@brarcher
Copy link
Contributor

I see that the Travis-CI build stopped working on the optional docs change. Does there need to be another example build to check that the docs can still be built, and that the docs can be skipped?

@mikkoi
Copy link
Contributor Author

mikkoi commented Aug 21, 2019

Commit "configure: optional build documentation" does add a new feature, even if it is only in build. @brarcher , can you add it to Travis?

@brarcher
Copy link
Contributor

The script which is run on Travis-CI is the travis.sh script. I'd suggest adding the following to the

"${USE_CMAKE}" = 'NO'

path around line 39:

autoreconf -i || exit 1
./configure ${EXTRA_ARGS} --disable-build-docs|| exit 1
make || exit 1

if [ -f doc/version.texi ]; then
echo "Documentation was generated, but was disabled";
exit 1;
fi

The rest of the test can proceed as normal.

Fix issue libcheck#206

Applied an uncommitted patch by Branden Archer.
https://sourceforge.net/p/check/bugs/101/

Signed-off-by: Mikko Johannes Koivunalho <mikko.koivunalho@iki.fi>
@mikkoi mikkoi force-pushed the is-makeinfo-required branch from a73137d to 486bddc Compare August 29, 2019 15:00
mikkoi added 3 commits August 29, 2019 18:27
Add more output when running `./configure`.

Signed-off-by: Mikko Johannes Koivunalho <mikko.koivunalho@iki.fi>
Tweak Travis build to support the new `./configure` switch.

Signed-off-by: Mikko Johannes Koivunalho <mikko.koivunalho@iki.fi>
libcheck#206

Signed-off-by: Mikko Johannes Koivunalho <mikko.koivunalho@iki.fi>
@mikkoi
Copy link
Contributor Author

mikkoi commented Sep 1, 2019

Missing the issue #217 .
Should we postpone?

@brarcher
Copy link
Contributor

brarcher commented Sep 3, 2019

If you would like to postpone merging this until both CMake and Autotools both have a configurable option to build the docs, that is fine. I'll leave it to your discretion. Just let me know when it is ready to merge.

@mikkoi
Copy link
Contributor Author

mikkoi commented Sep 4, 2019

The documentation itself needs to be revised. Part of the example needs to be rewritten so show how the new CMake import works. I have improved the file FindCheck.cmake to follow new CMake best practices. It is used to find Check installation from CMake build when the build does not support CMake style export/import or the using build wants to use an earlier (0.12.0 or earlier) Autotools built installation.
I think we should merge this branch now and keep the issue #217 open but close issue #206 .

@brarcher brarcher merged commit 93aacdf into libcheck:master Sep 8, 2019
@brarcher brarcher mentioned this pull request Sep 9, 2019
@mikkoi mikkoi deleted the is-makeinfo-required branch September 9, 2019 18:23
AC_MSG_WARN(tex not installed: cannot rebuild HTML documentation.)

if test "xtrue" = x"$enable_build_docs"; then
AC_CHECK_PROGS(TEX, tex, false)
Copy link
Contributor

@nmeum nmeum Oct 25, 2019

Choose a reason for hiding this comment

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

Why does this check for tex instead of makeinfo? The issue you referenced also mentions makeinfo, not tex. Your doc makefile also doesn't seem to call tex explicitly instead it uses the implicit GNU make rule which uses makeinfo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for pointing this out, and thanks for your fix in #231

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