Misc. build fixes encountered when cross-compiling #418

troglobit · 2023-02-07T17:38:01Z

We've set about to evaluate sysrepo vs Clixon for our little nework os, and Clixon is looking really awesome so far! :)

Wd when building the toolchain, example, and utils in Buildroot we ran into the following minor things. Please let me know if you want something done in another way.

olofhagsand

I wish we could do that. The problem now is that @@ expands only one step so that eg @sysconfdir@ expands to ${prefix}/etc/example.xml where ${prefix} is not evaluated further. And this is an XML file.
I dont know how to expand it all the way in the configure step?
Alternative would be to evaluate it in runtime/loadtime but that would mean evaluation of $prefix after parsing the XML. That is more complex and maybe even unsafe.
I noted that this was not covered in regression testing, and the main example is a greyzone wrt being part of the core package.
Nevertheless this needs to be fixed since ioto serves as the main example for others.

olofhagsand · 2023-02-08T15:00:24Z

util/Makefile.in

@@ -95,7 +95,9 @@ APPSRC   += clixon_util_yang.c
 APPSRC   += clixon_util_xpath.c
 APPSRC   += clixon_util_path.c
 APPSRC   += clixon_util_datastore.c
+ifdef with_libxml2


Actually this is too restrictive. clixon_util_regexp is needed in the regexp tests also for non-libxml2 regexps (ie posix regexps).
Preferably it should be enough with the HAVE_LIBXML2 define inside the code of clixon_util_regexp.c, so that only posix regexps are run.

OK, I'll fix that, no problem!

olofhagsand · 2023-02-08T15:20:09Z

configure.ac

@@ -228,6 +229,9 @@ if test "$ac_enable_publish" = "yes"; then
   AC_CHECK_HEADERS(curl.h,[], AC_MSG_ERROR([curl missing]))	
   AC_CHECK_LIB(curl, curl_global_init,, AC_MSG_ERROR([libcurl missing]))
   AC_DEFINE(CLIXON_PUBLISH_STREAMS, 1, [Enable publish of notification streams using SSE and curl])
+   with_curl=yes


The dependency here is wrong in the original configure.ac.
That is, curl is checked for only if --enable-publish is set.
But --enable-publish is a little-used option while the clxon_util_ applications are necessary for regression testing.
Therefore, the with-curl should be made independent of --enable-publish and also the dependencies on the regression testing should be sorted out, ie the places where clixon_util_stream etc are used in the tests.
My recommendation is to move this out of this PR and add a separate issue on this.

Yeah, I wanted to keep the diff as small as possible, but you're right I considered refactoring that whole thing first. OK, I'll break out a refactor as a separate PR.

OK, so this one turned out to be a lot simpler¹ than I first thought, so I'm including it in the same PR. The libcurl check only affects util/clixon_util_stream.c and lib/src/clixon_stream.c, the rest of the test system use the curl tool.

Footnotes

Unless I've missed something glaringly obvious. ↩

troglobit · 2023-02-08T15:25:58Z

I wish we could do that. The problem now is that @@ expands only one step so that eg @sysconfdir@ expands to ${prefix}/etc/example.xml where ${prefix} is not evaluated further. And this is an XML file. I dont know how to expand it all the way in the configure step? Alternative would be to evaluate it in runtime/loadtime but that would mean evaluation of $prefix after parsing the XML. That is more complex and mayebe even unsafe.

I tested it before I submitted, and it works in my cross-build setup, replacing ${prefix}, but now that I retest it again in a native build I see it. I've done workarounds for that before using eval in configure.ac in some of my own projects, so shouldn't be too difficult to get this sorted. We definitely wan to do it pre-runtime.

troglobit · 2023-02-08T15:28:44Z

Btw, what do you prefer: force-push to update this PR, or additional commits to see the review comments being addressed?

olofhagsand · 2023-02-08T15:34:26Z

We've set about to evaluate sysrepo vs Clixon for our little nework os, and Clixon is looking really awesome so far! :)

Thanks! It would be nice if you have something to share? For example some high-level comparison. We often get the question how clixon compares to other alternatives, but I see no public info.

Another info sharing would be wether clixon builds on buildroot and/or how to make it build.

Wd when building the toolchain, example, and utils in Buildroot we ran into the following minor things. Please let me know if you want something done in another way.

The patches as they were proposed work for the actual target build. However they broke the regression test scripts (and example as s showcase). As long as the regression is embedded inside clixon-core this is a problem.
Going forward, either (1)you alter the patches to not break the regression tests. Or (2) we take your observations as input and make patches that take the test scripts in account. (3)Moving test and example out of clixon is also an option (but more far-fetched) but maybe necessary in the long run.

troglobit · 2023-02-08T17:17:37Z

We've set about to evaluate sysrepo vs Clixon for our little nework os, and Clixon is looking really awesome so far! :)
Thanks! It would be nice if you have something to share? For example some high-level comparison. We often get the question how clixon compares to other alternatives, but I see no public info.

Good idea, I'll see what I can do!

Another info sharing would be wether clixon builds on buildroot and/or how to make it build.

When fully integrated and tested out that will be published here:

https://github.com/kernelkit/infix/tree/main/package/clixon

Currently only the initial packaging is done. We aim to do implement ietf-system and ietf-network yang modules to a certain extent and there will likely be more work on this packaging for that. When we're happy with the result we'll upstream packaging to Buildroot proper.

Wd when building the toolchain, example, and utils in Buildroot we ran into the following minor things. Please let me know if you want something done in another way.
The patches as they were proposed work for the actual target build. However they broke the regression test scripts (and example as s showcase). As long as the regression is embedded inside clixon-core this is a problem. Going forward, either (1)you alter the patches to not break the regression tests. Or (2) we take your observations as input and make patches that take the test scripts in account. (3)Moving test and example out of clixon is also an option (but more far-fetched) but maybe necessary in the long run.

I'm going for alt. (1).

Thank you so much for this initial review! <3

olofhagsand · 2023-02-08T19:00:25Z

Btw, what do you prefer: force-push to update this PR, or additional commits to see the review comments being addressed?

Not additional commits

This patch replaces the hard-coded `-I /usr/include/libxml2` used when building clixon_util_regexp with the output from `xml2-config --cflags`. To support cross-compiling, and preserve backwards compatibility with the `--with-libxml2` option, we allow the user to pass the path to the xml2-config tool as an optional argument. Similar to what python-lxml, and other packages, that rely on libxml2 do. The argument is optional to ensure that we default to use the hard-coded path from before. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>

Both util/clixon_util_stream.c and lib/src/clixon_stream.c depend on libcurl. The latter has `#ifdef CLIXON_PUBLISH_STREAMS` but the former does not. So `make util` fails without `--enable-publish` if libcurl is not installed. To preserve the original behavior (libcurl is an implicit dependency), this patch adds a `--without-libcurl` option to indicate this default. The check for libcurl and curl/curl.h is factored out as a separate check before checking for `--enable-publish`. If the two build options are in conflict we exit with a clear error code. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>

This is used everywhere else so looks loke a simple omission. Needed when cross-compiling (or packaging) Clixon. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>

Natvie build with GCC 11.3 generates the following warning. The patch is silly and the code path should never be reached, but it silences the compiler. restconf_main_native.c:572:9: warning: ‘addr’ may be used uninitialized in this function [-Wmaybe-uninitialized] Signed-off-by: Joachim Wiberg <troglobit@gmail.com>

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>

troglobit · 2023-02-09T15:34:24Z

Lots of action in this PR, I'll let you know when I lift the draft status, still testing out the changes I've made today (on a separate Ubuntu 18.04 machine).

troglobit · 2023-02-10T12:34:45Z

Sorry for the delay, I'm having trouble getting the regression tests to work properly. I've managed to run all of them except one that always fails for me, even on the master branch, but I can see that it does not fail in GitHub Actions. So I'm a bit curious if you could provide some insight into why test_restconf_internal.sh fails the way it does for me?

I've tried to figure out what differs between my Linux Mint 22.1 system (same as Ubuntu 22.04 with other GUI) and GitHub's ubuntu-latest runner, but I could not see anything obvious. So I tried setting up a VM with Ubuntu 18.04 (to get the earlier autoconf 2.69 used today for configure.ac), and even with a clean git clone of clixon master I get the following failure:

Running test_restconf_internal.sh
Error in test_restconf_internal.sh errcode=255
...
Test 39(39) [10. restart restconf RPC]
Test 40(40) [send rpc restart]
Test 41(41) [11. Get restconf status rpc]
Test 42(42) [send rpc status]
grep: warning: stray \ before -
grep: warning: stray \ before -
Test 43(43) [Kill backend]
Feb 10 12:27:07: Killing old daemon with pid: 60371
Test 44(44) [Restart backend -s none]
Test 45(45) [kill old backend]
Test 46(46) [start backend -s none -f /var/tmp/./test_restconf_internal.sh/conf.xml]
Test 47(47) [wait backend]
Test 48(48) [wait restconf]
speed 38400 baud; line = 0;
intr = ^C; quit = ^\; erase = ^?; kill = ^U; eof = ^D; eol = <undef>;
eol2 = <undef>; swtch = <undef>; start = ^Q; stop = ^S; susp = ^Z; rprnt = ^R;
werase = ^W; lnext = ^V; flush = ^O; min = 1; time = 0;
-brkint -imaxbel
Test 49(49) [12. Get restconf (running) after restart]
Test 50(50) [send rpc status]
grep: warning: stray \ before -
grep: warning: stray \ before -

Error in Test50 [send rpc status]:
Expected
^<rpc-reply xmlns="urn:ietf:params:xml:ns:netconf:base:1.0" message-id="42"><active xmlns="http://clicon.org/lib">true</active><description xmlns="http://clicon.org/lib">Clixon RESTCONF process</description><command xmlns="http://clicon.org/lib">/.*/clixon_restconf -f /var/tmp/./test_restconf_internal.sh/conf.xml -D [0-9] .*</command><status xmlns="http://clicon.org/lib">running</status><starttime xmlns="http://clicon.org/lib">20[0-9][0-9]\-[0-9][0-9]\-[0-9][0-9]T[0-9][0-9]:[0-9][0-9]:[0-9][0-9]\.[0-9]*Z</starttime><pid xmlns="http://clicon.org/lib">0</pid></rpc-reply>$

Received: 
#889
<rpc-reply xmlns="urn:ietf:params:xml:ns:netconf:base:1.0" message-id="42"><active xmlns="http://clicon.org/lib">false</active><description xmlns="http://clicon.org/lib">Clixon RESTCONF process</description><command xmlns="http://clicon.org/lib">/usr/local/sbin/clixon_restconf -f /var/tmp/./test_restconf_internal.sh/conf.xml -D 0 -l s -R &lt;restconf xmlns="http://clicon.org/restconf"&gt;&lt;enable&gt;true&lt;/enable&gt;&lt;auth-type&gt;none&lt;/auth-type&gt;&lt;debug&gt;0&lt;/debug&gt;&lt;log-destination&gt;syslog&lt;/log-destination&gt;&lt;enable-core-dump&gt;false&lt;/enable-core-dump&gt;&lt;pretty&gt;false&lt;/pretty&gt;&lt;socket&gt;&lt;namespace&gt;default&lt;/namespace&gt;&lt;address&gt;0.0.0.0&lt;/address&gt;&lt;port&gt;80&lt;/port&gt;&lt;ssl&gt;false&lt;/ssl&gt;&lt;/socket&gt;&lt;/restconf&gt;</command><status xmlns="http://clicon.org/lib">stopped</status></rpc-reply>
##

I've stripped down the output above, including complete-test.log as well for reference.

olofhagsand · 2023-02-10T21:30:38Z

test_restconf_internal.sh is somewhat sensitive to timing, so it may fail occasionally on some platforms, which is the action true vs false issue that I see in the trace above. It usually works fine in the docker test which is run in the regression (make test).
If that test fails natively, try to run it in docker.
We should probably put some work in making it more robust.
I see also some warnings regarding grep which I dont recognize?

troglobit · 2023-02-11T12:23:42Z

I've only run make test, which seems to always run things in an alpine docker. Is there a way to run the tests natively as well? Anyway the test fails consistently on all machines and OS' I've run it on so far. I'll dive in to the details of it and see what I can find. Thank you!

olofhagsand · 2023-02-11T15:03:02Z

native tests are run by building and install natively, cd to the test/ directory and running the tests from there, such as ./sum.sh
But this may be some work to set up the local environment appopriately.
The alpine docker tests are easier in that respect.

troglobit · 2023-02-11T15:18:47Z

Aha, of course, thank you! I'll give it a go in a dedicated VM.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>

troglobit · 2023-02-14T16:32:31Z

This was a tricky one, but with a bit of help from a colleague we figured out the problem with test_restconf_internal.sh, see issue #420.

With the "fixes" from there I've retested the updated patches I force-pushed last week and they pass OK now. So I'm removing the draft status of the PR. Thank you for your patience!

olofhagsand · 2023-02-14T18:23:01Z

There is some problem with the ci https://github.com/clicon/clixon/actions/runs/4135494336/jobs/7175071886

checking curl/curl.h usability... no
checking curl/curl.h presence... no
checking for curl/curl.h... no
configure: error: curl header(s) missing
Error: Process completed with exit code 1.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>

troglobit · 2023-02-14T19:39:02Z

My bad, I interpreted your "the clxon_util_ applications are necessary for regression testing." as clixon_util_stream.c being required, so I did not check if curl-dev was installed in the GitHub Action. Either I misunderstood or it's not required for the tests. I'll push another commit to update ci.yml

olofhagsand · 2023-02-14T19:52:31Z

it seems to be the action host not the docker, actually the config part of the host in .github/workflows/ci.yml is not really necessary, but needs to be done for the bootstrapping of the test container.

olofhagsand

Apart from the actions problem I have reviiewed the changes and run native tests without problems.
Thank you for your work.

troglobit · 2023-02-14T20:51:18Z

it seems to be the action host not the docker, actually the config part of the host in .github/workflows/ci.yml is not really necessary, but needs to be done for the bootstrapping of the test container.

Ah, I see. OK, since you approved the PR I guess you'll look into that yourself?

troglobit · 2023-02-14T20:52:26Z

Apart from the actions problem I have reviiewed the changes and run native tests without problems. Thank you for your work.

No, thank you for your patience and for Clixon! I expect we'll have more fixes and ideas coming soon.

olofhagsand · 2023-02-15T08:00:02Z

I guess you'll look into that yourself?

I guess I will

troglobit force-pushed the kkit branch from 7bbc0e1 to 265bd02 Compare February 8, 2023 08:59

olofhagsand requested changes Feb 8, 2023

View reviewed changes

olofhagsand reviewed Feb 8, 2023

View reviewed changes

troglobit marked this pull request as draft February 8, 2023 17:17

troglobit added 6 commits February 9, 2023 15:51

example/main: add missing $(DESTDIR) to standard include path

b48c6ad

This is used everywhere else so looks loke a simple omission. Needed when cross-compiling (or packaging) Clixon. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>

example/main: replace hard-coded path with variables from configure

9cd764f

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>

configure.ac: fix string comparison operator, == vs =

1283270

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>

troglobit force-pushed the kkit branch from 078c6db to 1283270 Compare February 9, 2023 14:55

troglobit mentioned this pull request Feb 14, 2023

Calling clixon_backend -z does not (always) stop clixon_restconf #420

Open

test: temporary fixes DROP BE BEFORE PUSH

e36a4e3

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>

troglobit marked this pull request as ready for review February 14, 2023 16:32

troglobit requested a review from olofhagsand February 14, 2023 16:33

.github: add curl-dev dependency for regression tests

058d3f7

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>

olofhagsand approved these changes Feb 14, 2023

View reviewed changes

olofhagsand merged commit ecd60fb into clicon:master Feb 17, 2023

troglobit deleted the kkit branch February 17, 2023 08:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Misc. build fixes encountered when cross-compiling #418

Misc. build fixes encountered when cross-compiling #418

troglobit commented Feb 7, 2023

olofhagsand left a comment •

edited

Loading

olofhagsand Feb 8, 2023

troglobit Feb 8, 2023

olofhagsand Feb 8, 2023

troglobit Feb 8, 2023

troglobit Feb 9, 2023

troglobit commented Feb 8, 2023

troglobit commented Feb 8, 2023

olofhagsand commented Feb 8, 2023

troglobit commented Feb 8, 2023

olofhagsand commented Feb 8, 2023

troglobit commented Feb 9, 2023

troglobit commented Feb 10, 2023

olofhagsand commented Feb 10, 2023 •

edited

Loading

troglobit commented Feb 11, 2023

olofhagsand commented Feb 11, 2023 •

edited

Loading

troglobit commented Feb 11, 2023

troglobit commented Feb 14, 2023

olofhagsand commented Feb 14, 2023

troglobit commented Feb 14, 2023

olofhagsand commented Feb 14, 2023 •

edited

Loading

olofhagsand left a comment

troglobit commented Feb 14, 2023

troglobit commented Feb 14, 2023

olofhagsand commented Feb 15, 2023

Misc. build fixes encountered when cross-compiling #418

Misc. build fixes encountered when cross-compiling #418

Conversation

troglobit commented Feb 7, 2023

olofhagsand left a comment • edited Loading

Choose a reason for hiding this comment

olofhagsand Feb 8, 2023

Choose a reason for hiding this comment

troglobit Feb 8, 2023

Choose a reason for hiding this comment

olofhagsand Feb 8, 2023

Choose a reason for hiding this comment

troglobit Feb 8, 2023

Choose a reason for hiding this comment

troglobit Feb 9, 2023

Choose a reason for hiding this comment

Footnotes

troglobit commented Feb 8, 2023

troglobit commented Feb 8, 2023

olofhagsand commented Feb 8, 2023

troglobit commented Feb 8, 2023

olofhagsand commented Feb 8, 2023

troglobit commented Feb 9, 2023

troglobit commented Feb 10, 2023

olofhagsand commented Feb 10, 2023 • edited Loading

troglobit commented Feb 11, 2023

olofhagsand commented Feb 11, 2023 • edited Loading

troglobit commented Feb 11, 2023

troglobit commented Feb 14, 2023

olofhagsand commented Feb 14, 2023

troglobit commented Feb 14, 2023

olofhagsand commented Feb 14, 2023 • edited Loading

olofhagsand left a comment

Choose a reason for hiding this comment

troglobit commented Feb 14, 2023

troglobit commented Feb 14, 2023

olofhagsand commented Feb 15, 2023

olofhagsand left a comment •

edited

Loading

olofhagsand commented Feb 10, 2023 •

edited

Loading

olofhagsand commented Feb 11, 2023 •

edited

Loading

olofhagsand commented Feb 14, 2023 •

edited

Loading