Created attachment 231913 [details] devel/xnvme: Cross-platform libraries and tools for NVMe devices This adds devel/xnvme as a new port. This port needs /usr/src to be built, but other than that it is straightforward. I have tested it on FreeBSD 13. --- xNVMe provides the means to program and interact with NMe devices from user space. The foundation of xNVMe is libxnvme, a user space library for working with NVMe devices. It provides a C API for memory management, that is, for allocating physical / DMA transferable memory when needed. xNVMe is an NVMe command interface allowing you to submit and complete NVMe commands in a synchronous as well as an asynchronous manner. WWW: https://xnvme.io Github: https://github.com/OpenMPDK/xNVMe
Thank you for your submission. I'm sorry it took so long to respond. Please check the following issues: - please check the indentation of the port Makefile. You can use the portfmt utility to format the port automatically. - is NASM really needed at runtime? - libraries should be depended on using LIB_DEPENDS, not RUN_DEPENDS. - For OpenSSL, use USES=ssl to have the ports framework pick the right dependency. - For ncurses, use USES=ncureses to have the ports framework pick the right dependency - why is git needed as a build dependency? Can it be removed? Many projects try to use git to determine some sort of commit ID for the build. This is broken when building from within the ports tree as the ports tree itself is managed using git and the script is likely to just pick up the current commit of the ports tree. If present, such scripts have to be patched to not check for a git commit. - the do-build and do-install targets are not needed. The builtin targets already do the right thing. - please move WWW from pkg-descr to Makefile. There has been a change in the way the project homepage is handled and we no longer accept ports using the old style. - please make sure to set maintainer-approval on your patches to make sure they get looked at quickly. Rest of the port looks okay. Can probably commit once these issues are addressed.
Created attachment 245464 [details] devel/xnvme: Cross-platform libraries and tools for NVMe devices This is the updated port. A lot has happened since the first submission, thus the port is a lot simpler. Let me know if any changes are needed. Thanks!
Comment on attachment 245464 [details] devel/xnvme: Cross-platform libraries and tools for NVMe devices Maintainer-approval should be set to "+" on ports you maintain or newly submit.
Thank you for your update. I've test-built the port and found some new issues. - Please use ${DISTVERSION} instead of hardcoding 0.7.1 in MASTER_SITES. This makes it easier to update the port. - Check if e.g. sysutils might be a more appropriate category for the port. Testing the port, it immediately fails to build due to a missing Python dependency: WARNING: Recommend using either -Dbuildtype or -Doptimization + -Ddebug. Using both is redundant since they override each other. See: https://mesonbuild.com/Builtin-options.html#build-type-options The Meson build system Version: 1.2.2 Source dir: /wrkdirs/usr/ports/devel/xnvme/work/xnvme-0.7.1 Build dir: /wrkdirs/usr/ports/devel/xnvme/work/xnvme-0.7.1/_build Build type: native build Project name: xnvme Project version: 0.7.1 C compiler for the host machine: cc (clang 14.0.5 "FreeBSD clang version 14.0.5 (https://github.com/llvm/llvm-project.git llvmorg-14.0.5-0-gc12386ae247c)") C linker for the host machine: cc ld.lld 14.0.5 Host machine cpu family: arm Host machine cpu: armv7 Message: host_machine.system: freebsd Compiler for C supports arguments -Wno-missing-braces: YES Compiler for C supports arguments -Wno-cast-function-type: YES Compiler for C supports arguments -Wno-strict-aliasing: YES Program python3 found: NO meson.build:49:31: ERROR: python3 not found A full log can be found at /wrkdirs/usr/ports/devel/xnvme/work/xnvme-0.7.1/_build/meson-logs/meson-log.txt WARNING: Running the setup command as `meson [options]` instead of `meson setup [options]` is ambiguous and deprecated. ===> Script "configure" failed unexpectedly. This is because on FreeBSD, we do not by default install a python3 binary and ports must not assume that it exist. To fix this, add "USES=python:build" to the port Makefile. This causes a PYTHON environment variable to be defined when meson is run. It holds the path of the Python interpreter. Obey that variable instead of assuming the command is named python3. Temporarily patching this issue by supplying pre-configure: ${REINPLACE_CMD} -e s,python3,${PYTHON_CMD}, ${WRKSRC}/meson.build in the port Makefile, the build then fails on armv7 FreeBSD 13.2 as follows: cc -Ilib/libxnvme.so.p -Ilib -I../lib -I. -I.. -Iinclude -I../include -fno-color-diagnostics -D_FILE_OFFSET_BITS=64 -Wa ll -Winvalid-pch -Wextra -std=gnu11 -Wno-missing-braces -Wno-cast-function-type -Wno-strict-aliasing -include xnvme_con fig.h -O2 -pipe -fstack-protector-strong -fno-strict-aliasing -fPIC -pthread -MD -MQ lib/libxnvme.so.p/xnvme_adm.c.o -M F lib/libxnvme.so.p/xnvme_adm.c.o.d -o lib/libxnvme.so.p/xnvme_adm.c.o -c ../lib/xnvme_adm.c In file included from ../lib/xnvme_adm.c:6: In file included from ../include/libxnvme.h:46: ../include/libxnvme_cmd.h:32:1: error: static_assert failed due to requirement 'sizeof(struct xnvme_cmd_ctx) == 128' "I ncorrect size" XNVME_STATIC_ASSERT(sizeof(struct xnvme_cmd_ctx) == 128, "Incorrect size") ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../include/libxnvme_util.h:16:40: note: expanded from macro 'XNVME_STATIC_ASSERT' #define XNVME_STATIC_ASSERT(cond, msg) static_assert(cond, msg); ^ ~~~~ /usr/include/assert.h:73:23: note: expanded from macro 'static_assert' #define static_assert _Static_assert ^ 1 error generated. It also fails elsewhere, but this is the first error. Please either fix the issue or disable armv7 (either with BROKEN_armv7 or NOT_FOR_ARCHS=armv7). On arm64 the build succeeds with a warning that should be addressed: [ 26% 54/201] cc -Ilib/libxnvme.so.p -Ilib -I../lib -I. -I.. -Iinclude -I../include -fno-color-diagnostics -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -std=gnu11 -Wno-missing-braces -Wno-cast-function-type -Wno-strict-aliasing -include xnvme_config.h -O2 -pipe -fstack-protector-strong -fno-strict-aliasing -fPIC -pthread -MD -MQ lib/libxnvme.so.p/xnvme_buf.c.o -MF lib/libxnvme.so.p/xnvme_buf.c.o.d -o lib/libxnvme.so.p/xnvme_buf.c.o -c ../lib/xnvme_buf.c ../lib/xnvme_buf.c:255:68: warning: format specifies type 'unsigned long long' but the argument has type 'size_t' (aka 'unsigned long') [-Wformat] printf(" - {byte: '%06llu', expected: 0x%x, actual: 0x%x)\n", i, exp[i], ~~~~~~ ^ %06zu 1 warning generated. Please check these issues and resubmit. I have so far not tested on x86 yet.
Other things you should check: - general policy is to install all shell completion files, even if the corresponding shells are not installed. Do not add dependencies on the shells you install completion files for. See § 6.33 Porter's Handbook for details. Shell completion files can be made optional, but that's only really needed if they are of substantial size. - add an option TEST and only install test binaries if this option is set. It seems like these binaries are not required for normal use, but feel free to correct me if I am wrong. - same thing applies to examples (with option EXAMPLES) - if you expect the library to be used separately from the tools, it might make sense to split the port into one port that only installs libraries and one that only installs tools and depends on the library port. - the port needs to set USE_LDCONFIG=yes as it installs shared libraries. - you may want to create a separate port for the Python bindings if desired - I see that you have extensive additional documentation. It would be great if you could add a DOCS option and install that documentation if it is enabled.
(In reply to Robert Clausecker from comment #5) Thanks Robert, I aim to address these things this week.
Created attachment 245527 [details] devel/xnvme: Cross-platform libraries and tools for NVMe devices I have attempted to address everything, however some things will come later. Specifically, the warning you see on arm64 has already been fixed upstream after 0.7.1 has been released. I am unsure what to do about completions, currently we only have Bash completions. Meson setup checks if bash is available on the system, to make sure that we install them in a sensible place. Perhaps we should change this to always install them here `${PREFIX}/share/bash-completion/completions` on FreeBSD. Do we need completions for Fish and ZSH as well? Finally, I have added options for the Tools, tests, examples etc., however this makes the pkg-plist inaccurate, since it only reflects the default. Is this okay, or is there something I need to do to fix this?
(In reply to Karl Bonde Torp from comment #7) Thank you for the update. Note that the build error (not warning) was on armv7, not arm64 (but I see that you disabled this platform). It also happens on i386. I recommend you check your code, I suspect you are using types with platform-specific length there. If you disable platforms, use NOT_FOR_ARCHS if there is a fundamental reason why it won't work (e.g. lack of platform-specific code) and then define NOT_FOR_ARCHS_REASON. If the reason is just some sort of bug or build error, set BROKEN_... instead. E.g. set BROKEN_armv7= sizeof(struct xnvme_cmd_ctx) != 128 This informs other people working on the ports tree that it may be worth investigating the problem. Perhaps someone then comes along and fixes it for you. > Perhaps we should change this to always install them here `${PREFIX}/share/bash-completion/completions` on FreeBSD. Yes, this would be correct for ports, however, it might not be correct for other users who want to install your software directly from source. Perhaps add an option to install the shell completion files and have that option default to whether bash is installed or not, but make it overridable. > Do we need completions for Fish and ZSH as well? It's not a requirement, but you are of course free to do so. > Finally, I have added options for the Tools, tests, examples etc., however this makes the pkg-plist inaccurate, since it only reflects the default. Is this okay, or is there something I need to do to fix this? The pkg-plist must be accurate for whatever options are set. Check § 5.14.3.1 Porter's Handbook for the OPTIONS_SUB mechanism designed to address this requirement. That said, note that options are a build-time thing. Most users download ports using binary package and won't use them. Hence my request if it was appropriate to split the libraries into a separate port, so they can be installed separately (and e.g. other ports can just depend on the libraries without also installing the tools). However, this is only needed if you expect 3rd party software to use the libraries. If you do not expect that, just having options is good enough. Note also that man pages should always be installed if the corresponding tools are installed (but please do not install man pages for tools that are not installed). I was requested a DOCS option for additional documentation, such as the one shipped in the docs directory of your project (apparently it's available on line at https://xnvme.io/docs/latest/). I also saw that you added a DEBUG option. While this option can be provided, it's usually meant for additional debug features. Just selecting a build type is done through WITH_DEBUG, and in fact, USES=meson does so automatically. No need to put an extra option in (see Mk/Uses/meson.mk).
(In reply to Robert Clausecker from comment #8) Hi Robert, > If you disable platforms, use NOT_FOR_ARCHS I think the way to go for the architectures is using an "ONLY_FOR_ARCHS= amd64", since this is the only thing we are currently testing - which is also why we haven't found the issues you're seeing. For both the Manpages and bash completions I will have to do some work upstream to improve the flexibility of our `meson.build` file. > The pkg-plist must be accurate for whatever options are set. In the latest patch I did put "OPTIONS_SUB= yes". The section § 5.14.3.1 Porter's Handbook is not very detailed, but it seems that I have done everything that is needed? > Hence my request if it was appropriate to split the libraries into a separate port I believe that installing both CLI tools and libraries is a sane default, and I don't believe there is any reason to split them up into two ports at the moment. > I was requested a DOCS option for additional documentation, such as the one shipped in the docs directory of your project As you point out, this is available online - and I don't think there is a need to include this in the port. Let's put this port on hold until xNVMe 0.7.2 is released, that will include the necessary fixes for the `meson.build` file. I will submit a new patch at that point, thanks for all your help!
(In reply to Karl Bonde Torp from comment #9) Hi Karl, > I think the way to go for the architectures is using an "ONLY_FOR_ARCHS= amd64", > since this is the only thing we are currently testing - which is also why we > haven't found the issues you're seeing. Please don't do that. If people were to generally only enable packages for architectures they personally tested, rare architectures like riscv64 would receive barely any packages at all. Only set ONLY_FOR_ARCHS if you know that the package will only work for the listed architectures. That you didn't test for others doesn't mean it won't work on these. You can always mark individual architectures as broken when problems are discovered, but unless known not to be the case, you should presume that your code works on any architecture. > For both the Manpages and bash completions I will have to do some work upstream to improve the flexibility of our `meson.build` file. Sounds great. We can temporarily commit the port without this flexibility and later update it with new options if you like. > In the latest patch I did put "OPTIONS_SUB= yes". The section § 5.14.3.1 Porter's Handbook is not very detailed, but it seems that I have done everything that is needed? That's half the game. This option causes placeholders like %%TESTS%% to be defined. You need to tag each file in pkg-plist with what option causes it to be installed by placing the placeholder right in front. Only files for which the appropriate option is on are then shipped. E.g. do %%TESTS%%bin/xnvme_tests_buf to only install this file in case the TESTS option is set. (For DOCS, the placeholder is %%PORTDOCS%% for historical reasons). It is okay if your build scripts install files that you don't want to keep, as long as they are listed for some (possibly currently disabled) option in pkg-plist. Such files won't be included in the package. However, it's of course better if you try to avoid doing so. > I believe that installing both CLI tools and libraries is a sane default, and I don't believe there is any reason to split them up into two ports at the moment. Okay, then let's go with that. > As you point out, this is available online - and I don't think there is a need to include this in the port. Sure, but please be mindful of users who lack internet access for some reason. Your tools look like they will be particularly useful for systems recovery and maintenance, which are typical scenarios where this might be the case. > I will submit a new patch at that point, thanks for all your help! Looking forwards to that and thanks for all the work.
Created attachment 246447 [details] devel/xnvme: Cross-platform libraries and tools for NVMe devices Hi Robert, xNVMe 0.7.2 has been released and I have created an updated port according to your feedback. It should be good to go in now, but let me know if something needs to be changed.
Thank you for the update. Testing with Poudriere, I find that the build immediately fails as your port depends on Python but does not declare that it does: ===> Configuring for xnvme-0.7.2 WARNING: Recommend using either -Dbuildtype or -Doptimization + -Ddebug. Using both is redundant since they override each other. See: https://mesonbuild.com/Builtin-options.html#build-type-options The Meson build system Version: 1.2.3 Source dir: /wrkdirs/usr/ports/sysutils/xnvme/work/xnvme-0.7.2 Build dir: /wrkdirs/usr/ports/sysutils/xnvme/work/xnvme-0.7.2/_build Build type: native build Project name: xnvme Project version: 0.7.2 C compiler for the host machine: cc (clang 14.0.5 "FreeBSD clang version 14.0.5 (https://github.com/llvm/llvm-project.git llvmorg-14.0.5-0-gc12386ae247c)") C linker for the host machine: cc ld.lld 14.0.5 Host machine cpu family: aarch64 Host machine cpu: aarch64 Message: host_machine.system: freebsd Compiler for C supports arguments -Wno-missing-braces: YES Compiler for C supports arguments -Wno-cast-function-type: YES Compiler for C supports arguments -Wno-strict-aliasing: YES Program python3 found: NO meson.build:49:31: ERROR: python3 not found A full log can be found at /wrkdirs/usr/ports/sysutils/xnvme/work/xnvme-0.7.2/_build/meson-logs/meson-log.txt WARNING: Running the setup command as `meson [options]` instead of `meson setup [options]` is ambiguous and deprecated. ===> Script "configure" failed unexpectedly. I think this is because you hard code prog_python = import('python').find_installation('python3') whereas we name the binary something like python3.9 or similar depending on the exact version of Python used. The environment variable PYTHON is set to the name of the Python interpreter when the build is configured. To fix this, it should suffice to just patch that line in meson.build to prog_python = import('python').find_installation() If I add this patch, the port builds fine. Are you ok with me doing so when checking in the package?
An extended build test fails on arm64 FreeBSD 12.4: FAILED: lib/libxnvme.so.p/xnvme_be_fbsd_async.c.o cc -Ilib/libxnvme.so.p -Ilib -I../lib -I. -I.. -Iinclude -I../include -fno-color-diagnostics -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -std=gnu11 -Wno-missing-braces -Wno-cast-function-type -Wno-strict-aliasing -include xnvme_config.h -O2 -pipe -fstack-protector-strong -fno-strict-aliasing -fPIC -pthread -MD -MQ lib/libxnvme.so.p/xnvme_be_fbsd_async.c.o -MF lib/libxnvme.so.p/xnvme_be_fbsd_async.c.o.d -o lib/libxnvme.so.p/xnvme_be_fbsd_async.c.o -c ../lib/xnvme_be_fbsd_async.c ../lib/xnvme_be_fbsd_async.c:263:9: error: no member named 'aio_iov' in 'struct aiocb' aiocb->aio_iov = dvec; ~~~~~ ^ ../lib/xnvme_be_fbsd_async.c:264:9: error: no member named 'aio_iovcnt' in 'struct aiocb' aiocb->aio_iovcnt = dvec_cnt; ~~~~~ ^ ../lib/xnvme_be_fbsd_async.c:274:9: warning: implicit declaration of function 'aio_writev' is invalid in C99 [-Wimplicit-function-declaration] err = aio_writev(aiocb); ^ ../lib/xnvme_be_fbsd_async.c:279:9: warning: implicit declaration of function 'aio_readv' is invalid in C99 [-Wimplicit-function-declaration] err = aio_readv(aiocb); ^ 2 warnings and 2 errors generated. FreeBSD 12 is only supported until the end of this year, so it would be ok to just mark the port as BROKEN_FreeBSD_12= no member named 'aio_iov' in 'struct aiocb' or if you want the port to work on FreeBSD 12, check if you can come up with a fix.
Created attachment 246460 [details] devel/xnvme: Cross-platform libraries and tools for NVMe devices Hi Robert, Thanks for the quick response! Nice catch on the python-thing, those two lines prog_python = import('python').find_installation('python3') message('python.language_version:', prog_python.language_version()) can just be removed, as we don't use them anymore - but somehow they survived when we removed the line actually using the python import. I have fixed it upstream - but no need to wait for the next release. It would be awesome if you create a patch for it. I have added the BROKEN_FreeBSD_12. Thanks!
Thank you for the quick response. Is Python now needed at all to build this port? If not, I would go ahead and remove the python dependency entirely.
fyi. if it makes it easier/patchless then xNVMe v0.7.3 is out, with the python-fix applied https://github.com/OpenMPDK/xNVMe/releases/tag/v0.7.3
Feel free to submit a new patch based on the updated version. Otherwise I'd go ahead and commit your patch as is this weekend. You can of course submit an update to your ports at any time.
Created attachment 246509 [details] devel/xnvme: Cross-platform libraries and tools for NVMe devices Hi Robert, I have created a (hopefully) final patch bumping the version to 0.7.3 and removing the uneeded python build dependency. Thank you for all your help!
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=e0e6669de4a7e8b01a66116cfa9b2305c2ed0b5b commit e0e6669de4a7e8b01a66116cfa9b2305c2ed0b5b Author: Karl Bonde Torp <k.torp@samsung.com> AuthorDate: 2023-10-09 11:46:15 +0000 Commit: Robert Clausecker <fuz@FreeBSD.org> CommitDate: 2023-11-24 06:12:46 +0000 sysutils/xnvme: Cross-platform libraries and tools for NVMe devices xNVMe provides the means to program and interact with NMe devices from user space. The foundation of xNVMe is libxnvme, a user space library for working with NVMe devices. It provides a C API for memory management, that is, for allocating physical / DMA transferable memory when needed. xNVMe is an NVMe command interface allowing you to submit and complete NVMe commands in a synchronous as well as an asynchronous manner. WWW: https://xnvme.io/ Signed-off-by: Karl Bonde Torp <k.torp@samsung.com> PR: 262032 sysutils/Makefile | 1 + sysutils/xnvme/Makefile (new) | 37 +++++++ sysutils/xnvme/distinfo (new) | 3 + sysutils/xnvme/pkg-descr (new) | 6 ++ sysutils/xnvme/pkg-plist (new) | 223 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 270 insertions(+)
Thank you for your contribution.