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

libcmd: update to support lowdown-1.4 API #12115

Merged
merged 1 commit into from
Dec 30, 2024

Conversation

trofi
Copy link
Contributor

@trofi trofi commented Dec 29, 2024

Motivation

Upstream change
kristapsdz/lowdown@bab1d75 moved a few fields from lowdown_opts toa new lowdown_opts_term struct. As a result the build started failing as:

nix-cmd> [2/17] Compiling C++ object libnixcmd.so.p/markdown.cc.o
nix-cmd> FAILED: libnixcmd.so.p/markdown.cc.o
nix-cmd> g++ -Ilibnixcmd.so.p -I. -I.. -I/nix/store/b0bnrk5lacxbpgxgnc28r8q3wcazrgxj-nix-util-2.26.0pre-dev/include/nix -I/nix/store/cxnynq9ykyj4xxv6wf6dw7r0aw5x6n9k-libarchive-3.7.7-dev/include -I/nix/store/bfgjwkcb8snkizx578rzdahi75m8zyh4-nlohmann_json-3.11.3/include -I/nix/store/3sx8bq3sip6j2nv1m5xx4gbdp33v7iy6-nix-store-2.26.0pre-dev/include/nix -I/nix/store/sih2dgqzvsbv7p510lkfmas7s7wbsl4j-nix-fetchers-2.26.0pre-dev/include/nix -I/nix/store/68p8s20fsiiakj7nys7grbaixfnhsdzs-nix-expr-2.26.0pre-dev/include/nix -I/nix/store/gw7wknhzhfzzj9zww2kyi5xrzgf1ndki-boehm-gc-8.2.8-dev/include -I/nix/store/3jwb9j4vnsk5saq3wfyyp9il3mhs41l9-nix-flake-2.26.0pre-dev/include/nix -I/nix/store/8nwjvmq7m48v8g646jrxkikv6x47bc3m-nix-main-2.26.0pre-dev/include/nix -I/nix/store/rb0hzsw5wc1a7daizhpj824mbxlvijrq-lowdown-1.4.0-dev/include -I/nix/store/m388ywpk53fsp8r98brfd7nf1f5sskv0-editline-1.17.1-dev/include -fdiagnostics-color=always -D_GLIBCXX_ASSERTIONS=1 -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -std=c++2a -include config-util.hh -include config-store.hh -include config-expr.hh -include config-main.hh -include config-cmd.hh -Wdeprecated-copy -Werror=suggest-override -Werror=switch -Werror=switch-enum -Werror=unused-result -Wignored-qualifiers -Wimplicit-fallthrough -Wno-deprecated-declarations -O3 -fPIC -pthread -std=c++2a -std=c++2a -std=c++2a -std=c++2a -std=c++2a -std=c++2a -MD -MQ libnixcmd.so.p/markdown.cc.o -MF libnixcmd.so.p/markdown.cc.o.d -o libnixcmd.so.p/markdown.cc.o -c ../markdown.cc
nix-cmd> ../markdown.cc: In function 'std::string nix::doRenderMarkdownToTerminal(std::string_view)':
nix-cmd> ../markdown.cc:28:5: error: 'lowdown_opts' has no non-static data member named 'cols'
nix-cmd>    28 |     };
nix-cmd>       |     ^

The change adds version-based conditional to support both pre-1.4 and 1.4 forms of the initialization.

Closes: #12113


Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@trofi trofi requested a review from edolstra as a code owner December 29, 2024 16:01
@darix
Copy link
Contributor

darix commented Dec 29, 2024

couldnt you just do something like

if lowdown.get_version.version_compare(">= 1.4")
   configdata.set('HAVE_LOWDOWN_1_4', lowdown.found().to_int())
endif

that would save you the 2nd finder?

https://mesonbuild.com/Syntax.html#if-statements
https://github.com/mesonbuild/meson/blob/master/mesonbuild/dependencies/base.py#L202

also will this be backported to the 2.24 branch?

@darix
Copy link
Contributor

darix commented Dec 29, 2024

also shouldnt the patch also fix the autoconf based build system?

@darix
Copy link
Contributor

darix commented Dec 29, 2024

Tested version (2.24.11)

diff --git a/src/libcmd/meson.build b/src/libcmd/meson.build
index c484cf998..2ae51a669 100644
--- a/src/libcmd/meson.build
+++ b/src/libcmd/meson.build
@@ -38,6 +38,10 @@ deps_public += nlohmann_json
 lowdown = dependency('lowdown', version : '>= 0.9.0', required : get_option('markdown'))
 deps_private += lowdown
 configdata.set('HAVE_LOWDOWN', lowdown.found().to_int())
+# The API changed slightly around terminal initialization.
+if lowdown.version().version_compare('>= 1.4')
+  configdata.set('HAVE_LOWDOWN_1_4', lowdown.found().to_int())
+endif
 
 readline_flavor = get_option('readline-flavor')
 if readline_flavor == 'editline'

Upstream change
kristapsdz/lowdown@bab1d75
moved a few fields from `lowdown_opts` toa  new `lowdown_opts_term`
struct. As a result the build started failing as:

    nix-cmd> [2/17] Compiling C++ object libnixcmd.so.p/markdown.cc.o
    nix-cmd> FAILED: libnixcmd.so.p/markdown.cc.o
    nix-cmd> g++ -Ilibnixcmd.so.p -I. -I.. -I/nix/store/b0bnrk5lacxbpgxgnc28r8q3wcazrgxj-nix-util-2.26.0pre-dev/include/nix -I/nix/store/cxnynq9ykyj4xxv6wf6dw7r0aw5x6n9k-libarchive-3.7.7-dev/include -I/nix/store/bfgjwkcb8snkizx578rzdahi75m8zyh4-nlohmann_json-3.11.3/include -I/nix/store/3sx8bq3sip6j2nv1m5xx4gbdp33v7iy6-nix-store-2.26.0pre-dev/include/nix -I/nix/store/sih2dgqzvsbv7p510lkfmas7s7wbsl4j-nix-fetchers-2.26.0pre-dev/include/nix -I/nix/store/68p8s20fsiiakj7nys7grbaixfnhsdzs-nix-expr-2.26.0pre-dev/include/nix -I/nix/store/gw7wknhzhfzzj9zww2kyi5xrzgf1ndki-boehm-gc-8.2.8-dev/include -I/nix/store/3jwb9j4vnsk5saq3wfyyp9il3mhs41l9-nix-flake-2.26.0pre-dev/include/nix -I/nix/store/8nwjvmq7m48v8g646jrxkikv6x47bc3m-nix-main-2.26.0pre-dev/include/nix -I/nix/store/rb0hzsw5wc1a7daizhpj824mbxlvijrq-lowdown-1.4.0-dev/include -I/nix/store/m388ywpk53fsp8r98brfd7nf1f5sskv0-editline-1.17.1-dev/include -fdiagnostics-color=always -D_GLIBCXX_ASSERTIONS=1 -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -std=c++2a -include config-util.hh -include config-store.hh -include config-expr.hh -include config-main.hh -include config-cmd.hh -Wdeprecated-copy -Werror=suggest-override -Werror=switch -Werror=switch-enum -Werror=unused-result -Wignored-qualifiers -Wimplicit-fallthrough -Wno-deprecated-declarations -O3 -fPIC -pthread -std=c++2a -std=c++2a -std=c++2a -std=c++2a -std=c++2a -std=c++2a -MD -MQ libnixcmd.so.p/markdown.cc.o -MF libnixcmd.so.p/markdown.cc.o.d -o libnixcmd.so.p/markdown.cc.o -c ../markdown.cc
    nix-cmd> ../markdown.cc: In function 'std::string nix::doRenderMarkdownToTerminal(std::string_view)':
    nix-cmd> ../markdown.cc:28:5: error: 'lowdown_opts' has no non-static data member named 'cols'
    nix-cmd>    28 |     };
    nix-cmd>       |     ^

The change adds version-based conditional to support both pre-1.4 and
1.4 forms of the initialization.

Closes: NixOS#12113
@trofi trofi force-pushed the libcmd-lowdown-1.4-support branch from 018a9c6 to edbfe86 Compare December 29, 2024 21:52
@trofi
Copy link
Contributor Author

trofi commented Dec 29, 2024

couldnt you just do something like

if lowdown.get_version.version_compare(">= 1.4")
   configdata.set('HAVE_LOWDOWN_1_4', lowdown.found().to_int())
endif

Sounds reasonable. I would prefer to set the value unconditionally to allow integer context in #if FOO to catch typos (as opposed to #ifdef FOO). Updated the patch to:

--- a/src/libcmd/meson.build
+++ b/src/libcmd/meson.build
@@ -38,2 +38,4 @@ deps_private += lowdown
 configdata.set('HAVE_LOWDOWN', lowdown.found().to_int())
+# The API changed slightly around terminal initialization.
+configdata.set('HAVE_LOWDOWN_1_4', lowdown.version().version_compare('>= 1.4.0').to_int())

also will this be backported to the 2.24 branch?

Probably not as is as autotools build system was removed from master.

@roberth roberth merged commit 61c3559 into NixOS:master Dec 30, 2024
12 checks passed
@roberth
Copy link
Member

roberth commented Dec 30, 2024

A backport to 2.24 would have to include autotools support, or you'd still be stuck on lowdown 1.3 only.
I'll trigger the automation anyway, if that's helpful.

@roberth roberth added the backport 2.24-maintenance Automatically creates a PR against the branch label Dec 30, 2024
mergify bot added a commit that referenced this pull request Dec 30, 2024
…2115

libcmd: update to support lowdown-1.4 API (backport #12115)
@darix
Copy link
Contributor

darix commented Dec 30, 2024

well yeah i tried to use meson with 2.24 in my package but that version of the meson build was quite incomplete compared to autotools and 2.25 still waits for libgit2 to be merged :)

@trofi trofi deleted the libcmd-lowdown-1.4-support branch December 30, 2024 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.24-maintenance Automatically creates a PR against the branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nix fails to build against lowdown-1.4.0: error: 'lowdown_opts' has no non-static data member named 'cols'
3 participants