<p><b>@b4n</b> requested changes on this pull request.</p>
<p>I'd think <code>make distcheck</code> should be failing without cmark in the environment</p><hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/747#discussion_r190768870">markdown/.gitignore</a>:</p>
<pre style='color:#555'>> @@ -1,2 +1 @@
-/peg-markdown/markdown_parser.c
-/peg-markdown/peg-0.1.9/leg
+
</pre>
<p>you could merely remove this file I believe</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/747#discussion_r190769159">markdown/Makefile.am</a>:</p>
<pre style='color:#555'>> @@ -1,11 +1,5 @@
include $(top_srcdir)/build/vars.auxfiles.mk
-SUBDIRS =
-
-if MARKDOWN_PEG_MARKDOWN
-SUBDIRS += peg-markdown
-endif
-
-SUBDIRS += src docs
+SUBDIRS = cmark src docs
</pre>
<p>I would rather not recurse inside <em>cmark</em> if it isn't enabled (as it was done for <em>peg-markdown</em>). But either works, so if you prefer this it's not a problem.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/747#discussion_r190769864">markdown/cmark/Makefile.am</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,43 @@
+if MD_BUILTIN_CMARK
+
+noinst_LTLIBRARIES = libcmark.la
+
+libcmark_la_SOURCES = \
+ blocks.c \
+ buffer.c \
+ buffer.h \
+ chunk.h \
+ cmark.c \
+ cmark_ctype.c \
+ cmark_ctype.h \
+ cmark.h \
</pre>
<p>you miss at least <em>cmark_version.h</em>, so it will not be included in the tarball (as it's not listed anywhere I can see). Check witt the <em>distcheck</em> make target, and obviously on an environment lacking the cmark library (or explicitly enabling the builtin copy)</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/747#discussion_r190773957">markdown/cmark/Makefile.am</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,43 @@
+if MD_BUILTIN_CMARK
+
+noinst_LTLIBRARIES = libcmark.la
+
+libcmark_la_SOURCES = \
+ blocks.c \
+ buffer.c \
+ buffer.h \
+ chunk.h \
+ cmark.c \
+ cmark_ctype.c \
+ cmark_ctype.h \
+ cmark.h \
</pre>
<p>you could generate the <em>cmark_version.h</em> automatically from a <em>cmark_version.h.in</em> fairly easily. Unfortunately the placeholder names are not namespaced properly (<code>PROJECT_</code>) so it wouldn't be too clean to replace them with the proper Autotools feature (which is sad, as it makes it super trivial, a matter of <code>AC_SUBST</code>ituing the macros and calling <code>AC_CONFIG_FILES([cmark/cmark_version.h])</code>), but you can do that with a <code>sed</code> call easily, something like this:</p>
<div class="highlight highlight-source-makefile"><pre><span class="pl-smi">CMARK_VERSION_MAJOR</span>=0
<span class="pl-smi">CMARK_VERSION_MINOR</span>=28
<span class="pl-smi">CMARK_VERSION_PATCH</span>=3
<span class="pl-smi">CLEANFILES</span> = cmark_version.h
<span class="pl-smi">EXTRA_DIST</span> = cmark_version.h.in
<span class="pl-en">cmark_version.h</span>: cmark_version.h.in Makefile
<span class="pl-s">$(<span class="pl-smi">SED</span>)</span> -e <span class="pl-s"><span class="pl-pds">'</span>s,[@]PROJECT_VERSION_MAJOR[@],$(CMARK_VERSION_MAJOR),g<span class="pl-pds">'</span></span> <span class="pl-cce">\</span>
-e <span class="pl-s"><span class="pl-pds">'</span>s,[@]PROJECT_VERSION_MINOR[@],$(CMARK_VERSION_MINOR),g<span class="pl-pds">'</span></span> <span class="pl-cce">\</span>
-e <span class="pl-s"><span class="pl-pds">'</span>s,[@]PROJECT_VERSION_PATCH[@],$(CMARK_VERSION_PATCH),g<span class="pl-pds">'</span></span> <span class="pl-cce">\ </span>
<span class="pl-k"><</span> <span class="pl-s">$(<span class="pl-smi">srcdir</span>)</span>/<span class="pl-c1">$@</span>.in <span class="pl-k">></span> <span class="pl-c1">$@</span></pre></div>
<p>Don't forget to check for <a href="https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/Particular-Programs.html#index-AC_005fPROG_005fSED-293" rel="nofollow"><code>AC_PROG_SED</code></a> in <em>markdown.m4</em> when the plugin is enabled (I would think it's already done somewhere (libtool?), but doing it for the plugin is safe(r))</p>
<p>Likewise, you could generate <em>cmark_export.h</em> to your needs (and if it has to include <em>stdbool.h</em> for a hack of some kind, so be it).</p>
<p>That's just suggestions for easing the update of the embedded library by minimizing and hopefully removing any difference. I would think their <em>cmark_version.h</em> file is auto-generated (or at least processed) anyway, so that should not create additional diversion.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/747#discussion_r190774831">markdown/cmark/config.h.in</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,76 @@
+#ifndef CMARK_CONFIG_H
+#define CMARK_CONFIG_H
</pre>
<p>this file seems useless in the current setup. However, some of its content could be moved to Autotools…</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/747#discussion_r190774880">markdown/cmark/config.h.in</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,76 @@
+#ifndef CMARK_CONFIG_H
+#define CMARK_CONFIG_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#cmakedefine HAVE_STDBOOL_H
</pre>
<p><code>AC_CHECK_HEADERS([stdbool.h])</code></p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/747#discussion_r190775168">markdown/cmark/config.h.in</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,76 @@
+#ifndef CMARK_CONFIG_H
+#define CMARK_CONFIG_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#cmakedefine HAVE_STDBOOL_H
+
+#ifdef HAVE_STDBOOL_H
+ #include <stdbool.h>
+#elif !defined(__cplusplus)
+ typedef char bool;
+#endif
</pre>
<p>Trickier, because Autotools doesn't really have a clean way of force-including stuff, and this file is a mere <em>config.h</em>… one way could be having a second <em>config.h</em> just for cmark, but I'm not sure how to manage this with stock Autotools, short of manually generating that one (e.g. having a template and performing the replacements or using <code>AC_SUBST</code>/<code>AC_CONFIG_FILES</code>).</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/747#discussion_r190775330">markdown/cmark/config.h.in</a>:</p>
<pre style='color:#555'>> +#ifndef CMARK_CONFIG_H
+#define CMARK_CONFIG_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#cmakedefine HAVE_STDBOOL_H
+
+#ifdef HAVE_STDBOOL_H
+ #include <stdbool.h>
+#elif !defined(__cplusplus)
+ typedef char bool;
+#endif
+
+#cmakedefine HAVE___BUILTIN_EXPECT
</pre>
<p>You might want to add a check for <code>__builtin_expect()</code> that <code>AC_DEFINE</code>s <code>HAVE___BUILTIN_EXPECT</code>. I'm not sure how you check for this, but an <code>AC_TRY_COMPILE</code> would do I guess.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/747#discussion_r190775346">markdown/cmark/config.h.in</a>:</p>
<pre style='color:#555'>> +
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#cmakedefine HAVE_STDBOOL_H
+
+#ifdef HAVE_STDBOOL_H
+ #include <stdbool.h>
+#elif !defined(__cplusplus)
+ typedef char bool;
+#endif
+
+#cmakedefine HAVE___BUILTIN_EXPECT
+
+#cmakedefine HAVE___ATTRIBUTE__
</pre>
<p>Same</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/747#discussion_r190775597">markdown/cmark/config.h.in</a>:</p>
<pre style='color:#555'>> +
+#ifdef HAVE_STDBOOL_H
+ #include <stdbool.h>
+#elif !defined(__cplusplus)
+ typedef char bool;
+#endif
+
+#cmakedefine HAVE___BUILTIN_EXPECT
+
+#cmakedefine HAVE___ATTRIBUTE__
+
+#ifdef HAVE___ATTRIBUTE__
+ #define CMARK_ATTRIBUTE(list) __attribute__ (list)
+#else
+ #define CMARK_ATTRIBUTE(list)
+#endif
</pre>
<p>Or instead of defining <code>HAVE___ATTRIBUTE__</code>, just <code>AC_DEFINE</code> <code>CMARK_ATTRIBUTE</code> as appropriate. Though, I'm not sure it's possible to define a macro taking arguments… at worse, just define a custom <code>CPPFLAG</code> playing with <code>-D</code> I guess</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/747#discussion_r190775697">markdown/cmark/config.h.in</a>:</p>
<pre style='color:#555'>> +
+#cmakedefine HAVE___ATTRIBUTE__
+
+#ifdef HAVE___ATTRIBUTE__
+ #define CMARK_ATTRIBUTE(list) __attribute__ (list)
+#else
+ #define CMARK_ATTRIBUTE(list)
+#endif
+
+#ifndef CMARK_INLINE
+ #if defined(_MSC_VER) && !defined(__cplusplus)
+ #define CMARK_INLINE __inline
+ #else
+ #define CMARK_INLINE inline
+ #endif
+#endif
</pre>
<p>similar here, you could check for availability and <code>AC_DEFINE</code> <code>CMARK_INLINE</code>, or use <code>-D</code></p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/747#discussion_r190775969">markdown/cmark/config.h.in</a>:</p>
<pre style='color:#555'>> + #define CMARK_ATTRIBUTE(list) __attribute__ (list)
+#else
+ #define CMARK_ATTRIBUTE(list)
+#endif
+
+#ifndef CMARK_INLINE
+ #if defined(_MSC_VER) && !defined(__cplusplus)
+ #define CMARK_INLINE __inline
+ #else
+ #define CMARK_INLINE inline
+ #endif
+#endif
+
+/* snprintf and vsnprintf fallbacks for MSVC before 2015,
+ due to Valentin Milea http://stackoverflow.com/questions/2915672/
+*/
</pre>
<p>I'm not sure if we support compiling with MSVC at all, but if it's the case and cmark requires properly behaving <code>[v]snprintf()</code> it might be important to have a similar thing.</p>
<hr>
<p>In <a href="https://github.com/geany/geany-plugins/pull/747#discussion_r190776803">markdown/cmark/Makefile.am</a>:</p>
<pre style='color:#555'>> + node.c \
+ node.h \
+ parser.h \
+ references.c \
+ references.h \
+ render.c \
+ render.h \
+ scanners.c \
+ scanners.h \
+ utf8.c \
+ utf8.h \
+ xml.c
+
+EXTRA_DIST = \
+ case_fold_switch.inc \
+ entities.inc
</pre>
<p>you also miss <em>scanners.re</em>.</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/geany/geany-plugins/pull/747#pullrequestreview-123218588">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABDrJyN5C0I_q1JXpH1l65P6pHvR3gphks5t12qCgaJpZM4ULx1l">mute the thread</a>.<img src="https://github.com/notifications/beacon/ABDrJ67ywmDeEdi00eMi1wt1RGN0qKvJks5t12qCgaJpZM4ULx1l.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">{"@context":"http://schema.org","@type":"EmailMessage","potentialAction":{"@type":"ViewAction","target":"https://github.com/geany/geany-plugins/pull/747#pullrequestreview-123218588","url":"https://github.com/geany/geany-plugins/pull/747#pullrequestreview-123218588","name":"View Pull Request"},"description":"View this Pull Request on GitHub","publisher":{"@type":"Organization","name":"GitHub","url":"https://github.com"}}</script>
<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/geany/geany-plugins","title":"geany/geany-plugins","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/geany/geany-plugins"}},"updates":{"snippets":[{"icon":"PERSON","message":"@b4n requested changes on #747"}],"action":{"name":"View Pull Request","url":"https://github.com/geany/geany-plugins/pull/747#pullrequestreview-123218588"}}}</script>
<script type="application/ld+json">{
"@type": "MessageCard",
"@context": "http://schema.org/extensions",
"hideOriginalBody": "false",
"originator": "37567f93-e2a7-4e2a-ad37-a9160fc62647",
"title": "@b4n requested changes on 747",
"sections": [
{
"text": "I'd think `make distcheck` should be failing without cmark in the environment",
"activityTitle": "**Colomban Wendling**",
"activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png",
"activitySubtitle": "@b4n",
"facts": [
]
}
],
"potentialAction": [
{
"targets": [
{
"os": "default",
"uri": "https://github.com/geany/geany-plugins/pull/747#pullrequestreview-123218588"
}
],
"@type": "OpenUri",
"name": "View on GitHub"
},
{
"name": "Unsubscribe",
"@type": "HttpPOST",
"target": "https://api.github.com",
"body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 338632037\n}"
}
],
"themeColor": "26292E"
}</script>