Search code, repositories, users, issues, pull requests...

utils/docker/prepare-for-build.sh, line 4 at r1 (raw file):

set(VERSION_MINOR 2)
set(VERSION_PATCH 0)
#set(VERSION_PRERELEASE rc1)

I'm not sure, but I believe we never used it before in any other repo...

codecov.yml, line 7 at r1 (raw file):

        threshold: 0.2

  ignore:

src/valgrind/ and src/sys I guess

README.md, line 4 at r1 (raw file):

[![Travis build status](https://travis-ci.org/pmem/rpma.svg?branch=master)](https://travis-ci.org/pmem/rpma)
[![GHA build status](https://github.com/pmem/rpma/workflows/RPMA/badge.svg)](https://github.com/pmem/rpma/actions)

branch=master

doc/librpma.7.md, line 11 at r1 (raw file):

[comment]: <> (SPDX-License-Identifier: BSD-3-Clause)
[comment]: <> (Copyright 2019, Intel Corporation)

2019?

doc/librpma.7.md, line 70 at r1 (raw file):

**XXX**(3)
and **<http://pmem.io>**

https (actually you can check the whole PR for "http://"

doc/librpma.Doxyfile.in, line 183 at r1 (raw file):

#---------------------------------------------------------------------------
# Configuration options related to the HTML output
#---------------------------------------------------------------------------

consider also man output (http://www.doxygen.nl/manual/starting.html#man_out)

utils/check_whitespace, line 4 at r1 (raw file):

#
# SPDX-License-Identifier: BSD-3-Clause
# Copyright 2015-2018, Intel Corporation

-2020 (you touched it this year, e.g. by changing the license ;) )

utils/docker/build.sh, line 44 at r1 (raw file):

fi

imageName=${DOCKERHUB_REPO}:0.1-${OS}-${OS_VER}

in CMake you have "0.2", make it consistent, I believe
you're missing this "0.1" prefix in other places (perhaps not only in this but also in other file(s) )

utils/docker/build.sh, line 67 at r1 (raw file):

# Only run doc update on pmem/rpma master branch
if [[ "$TRAVIS_BRANCH" != "master" || "$TRAVIS_PULL_REQUEST" != "false" || "$TRAVIS_REPO_SLUG" != "${GITHUB_REPO}" ]]; then

consider the latest changes (already to include 'valid branches', see e.g. pmem/libpmemobj-cpp#656 )

#
# SPDX-License-Identifier: BSD-3-Clause
# Copyright 2019, Intel Corporation

2020

utils/docker/pull-or-rebuild-image.sh, line 12 at r1 (raw file):

#
# The script rebuilds the Docker image if the Dockerfile for the current
# OS version (Dockerfile.${OS}-${OS_VER}) or any .sh script from the directory

as mentioned in prev. comment - missing "0.1" prefix

utils/docker/run-build.sh, line 79 at r1 (raw file):

}

cd $WORKDIR

this is redundant I believe

utils/docker/run-coverity.sh, line 4 at r1 (raw file):

#
# SPDX-License-Identifier: BSD-3-Clause
# Copyright 2018-2019, Intel Corporation

.

utils/md2man/default.man, line 21 at r1 (raw file):

$endfor$
.\" SPDX-License-Identifier: BSD-3-Clause
.\" Copyright 2019-$year$, Intel Corporation

2019?

utils/md2man/md2man.sh, line 4 at r1 (raw file):

#
# SPDX-License-Identifier: BSD-3-Clause
# Copyright 2020, Intel Corporation

I guess this script is a bit older than just 2020 ;)

janekmi

Reviewed 6 of 76 files at r1.
Reviewable status: 21 of 76 files reviewed, 23 unresolved discussions (waiting on @ldorau)

utils/check_license/check-headers.sh, line 69 at r1 (raw file):

sudo: required

language: cpp

?

ChangeLog, line 5 at r1 (raw file):

	* Version 1.0

	This is the first release of librpma as a separate project.

I think we will add this file when rpma will have a release.

doc/librpma.Doxyfile.in, line 20 at r1 (raw file):

# The default value is: My Project.

PROJECT_NAME = "librpma"

RPMA

doc/librpma.Doxyfile.in, line 109 at r1 (raw file):

# *.qsf, *.as and *.js.

FILE_PATTERNS = "*.c *.h"

You are sure this is the right way to concat patterns? I cannot find this in the documentation.

examples/README, line 2 at r1 (raw file):

This directory contains examples for librpma, the library providing
a transactional object store for pmem.  Some of these examples are explained

This directory contains examples for librpma, the library to simplify accessing persistent memory devices on remote hosts over Remote Direct Memory Access (RDMA).

examples/README, line 3 at r1 (raw file):

This directory contains examples for librpma, the library providing
a transactional object store for pmem.  Some of these examples are explained
in more detail here: https://pmem.io/librpma/

Remove this sentence. It is enough what we have in the last paragraph.

examples/README, line 7 at r1 (raw file):

If you're looking for documentation to get you started using PMDK,
start here: https://pmem.io/pmdk and follow the links to examples and
man pages.

using RPMA
start here: https://pmem.io/librpma/

ldorau · 2020-02-13T10:45:28Z

My builds:

Travis: https://travis-ci.org/ldorau/rpma/builds
GHA: https://github.com/ldorau/rpma/actions

janekmi

Reviewed 7 of 76 files at r1.
Reviewable status: 24 of 76 files reviewed, 26 unresolved discussions (waiting on @janekmi, @ldorau, and @lukaszstolarczuk)

utils/cppstyle, line 46 at r1 (raw file):

foreach(@ARGV) {
	check($_)
}

We do not plan to have a c++ code here. I think we can get rid of it.

	[ -z $MERGE_BASE ] && \
		MERGE_BASE=$($GIT log --pretty="%cN:%H" | grep GitHub | head -n1 | cut -d: -f2)
	[ -z "$MERGE_BASE" -o "$CURRENT_COMMIT" == "$MERGE_BASE" ] && \

I have found in this line a difference comparing to what is available on PMDK.
It looks like small improvements/fixes. Do you know why it is not present also on PMDK?

utils/docker/images/README.md, line 22 at r1 (raw file):

```sh
docker run --network=bridge --shm-size=4G -v /your/workspace/path/:/opt/workspace:z -w /opt/workspace/ -e CC=clang -e CXX=clang++ -e PKG_CONFIG_PATH=/opt/pmdk/lib/pkgconfig -it rpma:debian-unstable /bin/bash

CXX?

marcinslusarz

Reviewed 4 of 76 files at r1.
Reviewable status: 25 of 76 files reviewed, 46 unresolved discussions (waiting on @janekmi, @ldorau, and @lukaszstolarczuk)

ChangeLog, line 5 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I think we will add this file when rpma will have a release.

+1

README.md, line 6 at r2 (raw file):

[![GHA build status](https://github.com/pmem/rpma/workflows/RPMA/badge.svg)](https://github.com/pmem/rpma/actions)
[![librpma version](https://img.shields.io/github/tag/pmem/rpma.svg)](https://github.com/pmem/rpma/releases/latest)
[![Coverity Scan Build Status](https://scan.coverity.com/projects/15911/badge.svg)](https://scan.coverity.com/projects/pmem-librpma)

Please drop Coverity badge for now

cmake/FindLIBIBVERBS.cmake, line 6 at r2 (raw file):

#

message(STATUS "Looking for libibverbs ...")

you should not need these Find*.cmake files. Use pkg-config.

cmake/librpma-config.cmake.in, line 8 at r2 (raw file):

@PACKAGE_INIT@

find_path(LIBPMEM_INCLUDE_DIR libpmem.h)

libpmem?

cmake/librpma.pc.in, line 5 at r2 (raw file):


Name: librpma
Description: librpma - template for a new libpmem library

libpmem?

cmake/librpma.pc.in, line 8 at r2 (raw file):

Version: @VERSION@
URL: https://github.com/pmem/rpma
Requires: libpmem >= @LIBPMEM_REQUIRED_VERSION@

no, that's not true

cmake/packages.cmake, line 37 at r2 (raw file):

set(CPACK_PACKAGE_VERSION_MAJOR ${VERSION_MAJOR})
set(CPACK_PACKAGE_VERSION_MINOR ${VERSION_MINOR})
set(CPACK_PACKAGE_DESCRIPTION_SUMMARY "template for a new libpmem library")

.

cmake/packages.cmake, line 44 at r2 (raw file):

set(CPACK_RPM_PACKAGE_LICENSE "BSD")
set(CPACK_RPM_PACKAGE_ARCHITECTURE x86_64)
set(CPACK_RPM_PACKAGE_REQUIRES "libpmem >= ${LIBPMEM_REQUIRED_VERSION}")

.

cmake/packages.cmake, line 50 at r2 (raw file):

set(CPACK_DEBIAN_PACKAGE_VERSION ${CPACK_PACKAGE_VERSION})
set(CPACK_DEBIAN_PACKAGE_ARCHITECTURE amd64)
set(CPACK_DEBIAN_PACKAGE_DEPENDS "libpmem (>= ${LIBPMEM_REQUIRED_VERSION})")

.

cmake/packages.cmake, line 51 at r2 (raw file):

set(CPACK_DEBIAN_PACKAGE_ARCHITECTURE amd64)
set(CPACK_DEBIAN_PACKAGE_DEPENDS "libpmem (>= ${LIBPMEM_REQUIRED_VERSION})")
set(CPACK_DEBIAN_PACKAGE_MAINTAINER "marcin.slusarz@intel.com")

no

doc/CMakeLists.txt, line 28 at r2 (raw file):

include(FindDoxygen)
if(DOXYGEN_FOUND AND DOXYGEN_DOT_FOUND)

are you going to provide doxygen documentaion?

doc/librpma.Doxyfile.in, line 1 at r2 (raw file):

#---------------------------------------------------------------------------

again, do we need this file at all?

src/librpma.map, line 6 at r2 (raw file):

#
#
# src/librpma.link -- linker link file for librpma

doesn't match the file name

/*

do you really need this file? I'm pretty sure you won't

tests/cmake/ctest_helpers.cmake, line 42 at r2 (raw file):

/*

I think Valgrind headers won't be needed.

tests/CMakeLists.txt, line 19 at r2 (raw file):

	# disable warning C4996 - disable checking iterators
	add_flag(-D_SCL_SECURE_NO_WARNINGS)

does it apply to C code?

tests/CMakeLists.txt, line 25 at r2 (raw file):

	# fix C1128 raised for some test binaries
	add_flag(-bigobj)

.

tests/valgrind_internal.h, line 1 at r2 (raw file):

/*

again, shouldn't be needed

endfunction()

function(find_pmemcheck)

.

tests/cmake/ctest_helpers.cmake, line 126 at r2 (raw file):

endfunction()

set(vg_tracers memcheck helgrind drd pmemcheck)

.

utils/check_license/check-ms-license.pl, line 1 at r2 (raw file):

#!/usr/bin/perl -w

this file shouldn't be needed at all

janekmi

Reviewed 9 of 76 files at r1.
Reviewable status: 29 of 76 files reviewed, 57 unresolved discussions (waiting on @janekmi, @ldorau, and @lukaszstolarczuk)

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

I'm not sure, but I believe we never used it before in any other repo...

wip?
We currently do not want to mark anything as ready in any respect.

CMakeLists.txt, line 11 at r2 (raw file):

set(VERSION_MAJOR 0)
set(VERSION_MINOR 2)

1

utils/check_license/file-exceptions.sh, line 7 at r1 (raw file):

set(LIBPMEM_REQUIRED_VERSION 1.7)
set(LIBIBVERBS_REQUIRED_VERSION 1.4)
set(LIBRDMACM_REQUIRED_VERSION 1.2)

We have to rethink these requirements in the near future.
What was your inspiration to set these exact values? :-)

CMakeLists.txt, line 29 at r2 (raw file):

set(CMAKE_DISABLE_IN_SOURCE_BUILD ON)

include(FindPerl)

?

CMakeLists.txt, line 31 at r2 (raw file):

include(FindPerl)
include(FindThreads)
include(CMakeDependentOption)

Not used?

CMakeLists.txt, line 32 at r2 (raw file):

include(FindThreads)
include(CMakeDependentOption)
include(CMakePackageConfigHelpers)

.?

CMakeLists.txt, line 33 at r2 (raw file):

include(CMakeDependentOption)
include(CMakePackageConfigHelpers)
include(CheckCSourceCompiles)

.?

CMakeLists.txt, line 34 at r2 (raw file):

include(CMakePackageConfigHelpers)
include(CheckCSourceCompiles)
include(CheckCCompilerFlag)

.?

# file-exceptions.sh - filter out files not checked for copyright and license

grep -v -E -e '/queue.h$' -e 'src/valgrind/'

$?

utils/docker/run-build.sh, line 19 at r2 (raw file):

PREFIX=/usr
TEST_DIR=${RPMA_TEST_DIR:-${DEFAULT_TEST_DIR}}
CHECK_CPP_STYLE=${CHECK_CPP_STYLE:-ON}

CPP?

utils/docker/run-build.sh, line 93 at r2 (raw file):

	-DCMAKE_INSTALL_PREFIX=$PREFIX \
	-DCOVERAGE=$COVERAGE \
	-DCHECK_CPP_STYLE=${CHECK_CPP_STYLE} \

.

utils/docker/run-doc-update.sh, line 41 at r2 (raw file):

git clean -df
cp -f $CURR_DIR/doc/*.md master/manpages/
cp -fr $CURR_DIR/cpp_html/* master/doxygen/

cpp?

ldorau

Reviewable status: 22 of 75 files reviewed, 57 unresolved discussions (waiting on @janekmi, @ldorau, @lukaszstolarczuk, and @marcinslusarz)

Previously, janekmi (Jan Michalski) wrote…

?

Done.

doc/librpma.7.md, line 11 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

2019?

Done.

doc/librpma.7.md, line 70 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

https (actually you can check the whole PR for "http://"

Done.

doc/librpma.Doxyfile.in, line 20 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

RPMA

Done.

doc/librpma.Doxyfile.in, line 109 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

You are sure this is the right way to concat patterns? I cannot find this in the documentation.

Done.

examples/README, line 2 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

This directory contains examples for librpma, the library to simplify accessing persistent memory devices on remote hosts over Remote Direct Memory Access (RDMA).

Done.

examples/README, line 3 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Remove this sentence. It is enough what we have in the last paragraph.

Done.

examples/README, line 7 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

using RPMA

start here: https://pmem.io/librpma/

Done.

src/librpma.map, line 6 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

doesn't match the file name

Done.

Previously, marcinslusarz (Marcin Ślusarz) wrote…

do you really need this file? I'm pretty sure you won't

It is needed by the current POC implementation of RPMA

Previously, marcinslusarz (Marcin Ślusarz) wrote…

I think Valgrind headers won't be needed.

@janekmi really?

tests/CMakeLists.txt, line 19 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

does it apply to C code?

Done.

tests/CMakeLists.txt, line 25 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

.

Done.

utils/check_whitespace, line 4 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

-2020 (you touched it this year, e.g. by changing the license ;) )

Done.

janekmi

Reviewed 5 of 76 files at r1, 2 of 21 files at r3.
Reviewable status: 28 of 75 files reviewed, 57 unresolved discussions (waiting on @janekmi, @ldorau, @lukaszstolarczuk, and @marcinslusarz)

Previously, ldorau (Lukasz Dorau) wrote…

Done.

?

ChangeLog, line 3 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

1.0 already?

We are progressing fast!

cmake/functions.cmake, line 13 at r3 (raw file):

	string(REPLACE ";" "${SEP}" JOIN_TMP "${VALUES}")
	set(${OUT} "${JOIN_TMP}" PARENT_SCOPE)
endfunction()

Not used.

cmake/functions.cmake, line 67 at r3 (raw file):

# instead.
# ${name} must be unique.
function(add_cppstyle name)

cpp

doc/librpma.Doxyfile.in, line 109 at r1 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

Done.

Can you refer the doxy documentation on it? I do not know it is right either way.

doc/librpma.Doxyfile.in, line 1 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

again, do we need this file at all?

We agreed to use doxygen documentation in this repo.
We hope to take this way:

inlined documentation
documentation also for static functions (this way it is easier to understand the implementation for newcomers; it may be useful if RPMA is also a reference implementation for other solutions)
easy (more or less) to generate man's from it too

utils/docker/images/install-pmdk.sh, line 34 at r3 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

@janekmi really?

We are using valgrind macros in librpmem. I'm pretty sure it will be also needed for RPMA.

VALGRIND_DO_MAKE_MEM_DEFINED
VALGRIND_ANNOTATE_IGNORE_READS_BEGIN/_END
VALGRIND_ANNOTATE_IGNORE_WRITES_BEGIN/_END

I think we can start with leaving headers at least headers which define these macros. Others we will add if needed.

	if [ "$PACKAGE_TYPE" = "dpkg" ]; then
		sudo dpkg -i dpkg/libpmem_*.deb dpkg/libpmem-dev_*.deb
		sudo dpkg -i dpkg/libpmemobj_*.deb dpkg/libpmemobj-dev_*.deb

You have skipped pmreorder intenionally?

utils/docker/images/push-image.sh, line 45 at r3 (raw file):

# Push the image to the repository
docker push ${DOCKERHUB_REPO}:0.1-$1

All these version strings are typed by hand everywhere in our repos, really?

ldorau

Reviewable status: 28 of 75 files reviewed, 57 unresolved discussions (waiting on @janekmi, @ldorau, @lukaszstolarczuk, and @marcinslusarz)

ChangeLog, line 3 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

1.0 already?

Done.

ChangeLog, line 5 at r1 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

+1

Done.

Previously, janekmi (Jan Michalski) wrote…

wip?
We currently do not want to mark anything as ready in any respect.

Done.

CMakeLists.txt, line 11 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

1

Done.

Previously, janekmi (Jan Michalski) wrote…

We have to rethink these requirements in the near future.
What was your inspiration to set these exact values? :-)

I have changed them to the current versions

CMakeLists.txt, line 29 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

?

utils/cppstyle requires Perl.
utils/cppstyle script is a wrapper for clang-format. We use clang-format instead of PMDK's cstyle perl script

codecov.yml, line 7 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

src/valgrind/ and src/sys I guess

Done.

README.md, line 4 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

branch=master

Done.

README.md, line 6 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

Please drop Coverity badge for now

Done.

cmake/FindLIBIBVERBS.cmake, line 6 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

you should not need these Find*.cmake files. Use pkg-config.

They are needed, because those packages do not have pkgconfig files on Ubuntu and Fedora.

cmake/librpma-config.cmake.in, line 8 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

libpmem?

Done.

cmake/librpma.pc.in, line 5 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

libpmem?

Done.

cmake/packages.cmake, line 37 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

.

Done.

cmake/packages.cmake, line 51 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

no

Removed

doc/CMakeLists.txt, line 28 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

are you going to provide doxygen documentaion?

Yes.

doc/librpma.Doxyfile.in, line 1 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

We agreed to use doxygen documentation in this repo.
We hope to take this way:

inlined documentation

documentation also for static functions (this way it is easier to understand the implementation for newcomers; it may be useful if RPMA is also a reference implementation for other solutions)

easy (more or less) to generate man's from it too

Yes.

Previously, janekmi (Jan Michalski) wrote…

We are using valgrind macros in librpmem. I'm pretty sure it will be also needed for RPMA.

VALGRIND_DO_MAKE_MEM_DEFINED

VALGRIND_ANNOTATE_IGNORE_READS_BEGIN/_END

VALGRIND_ANNOTATE_IGNORE_WRITES_BEGIN/_END

I think we can start with leaving headers at least headers which define these macros. Others we will add if needed.

Done.

tests/valgrind_internal.h, line 1 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

again, shouldn't be needed

Removed

janekmi

Reviewed 2 of 76 files at r1, 4 of 21 files at r3, 3 of 15 files at r4.
Reviewable status: 31 of 70 files reviewed, 58 unresolved discussions (waiting on @janekmi, @ldorau, @lukaszstolarczuk, and @marcinslusarz)

.codecov.yml, line 7 at r4 (raw file):

  - examples/
  - utils/
  - tests/

A-Z order?
cmake?

Previously, ldorau (Lukasz Dorau) wrote…

Done.

set(VERSION_PATCH wip)

It is possible?

utils/docker/images/Dockerfile.fedora-31, line 12 at r4 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

I have changed them to the current versions

My ubuntu has libibverbs 1.6. ;-)

# Pull base image
FROM fedora:31
MAINTAINER firstname.secondname@intel.com

@grom72 @marcinslusarz ?

utils/docker/images/Dockerfile.fedora-31, line 49 at r4 (raw file):

	wget \
	which \
	xmlto \

.

utils/docker/images/Dockerfile.ubuntu-19.10, line 12 at r3 (raw file):

# Pull base image
FROM ubuntu:19.10
MAINTAINER firstname.secondname@intel.com

@grom72 @marcinslusarz: any candidates to take this place?

utils/docker/images/Dockerfile.ubuntu-19.10, line 48 at r3 (raw file):

	uuid-dev \
	wget \
	whois \

We are really using all of this?

janekmi

Reviewed 6 of 76 files at r1, 1 of 21 files at r3, 5 of 15 files at r4.
Reviewable status: 42 of 70 files reviewed, 58 unresolved discussions (waiting on @janekmi, @ldorau, @lukaszstolarczuk, and @marcinslusarz)

cmake/FindLIBIBVERBS.cmake, line 6 at r2 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

They are needed, because those packages do not have pkgconfig files on Ubuntu and Fedora.

Are you sure? My ubuntu responds properly:

$ pkg-config libibverbs --moversion
1.6.24.0

cmake/librpma.pc.in, line 8 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

no, that's not true

👍

Requires:

libibverbs
librdmacm

examples/template-example/CMakeLists.txt, line 7 at r4 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

Done.

Nope.

My grepping is telling these macros were defined in valgrind_internal.h.
I guess we need that one. I'm not sure why we would need drd.h, helgrind.h, memcheck.h.
Any arguments here?

janekmi

Reviewed 13 of 76 files at r1, 3 of 3 files at r2, 8 of 21 files at r3, 5 of 15 files at r4.
Reviewable status: 67 of 70 files reviewed, 72 unresolved discussions (waiting on @janekmi, @ldorau, @lukaszstolarczuk, and @marcinslusarz)

doc/librpma.7.md, line 7 at r4 (raw file):

collection: librpma
header: PMDK
date: rpma API version 1.3

0.1 at best. ;-)

doc/librpma.Doxyfile.in, line 183 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

consider also man output (http://www.doxygen.nl/manual/starting.html#man_out)

👍

examples/CMakeLists.txt, line 7 at r4 (raw file):

if(MSVC_VERSION)
	add_flag(-W4)

No Windows here.

examples/CMakeLists.txt, line 37 at r4 (raw file):

include_directories(
	${LIBRPMA_INCLUDE_DIRS}
	${CMAKE_CURRENT_SOURCE_DIR}/../src

Includes should be stored only in the include directory.
Please remove src from here.

examples/CMakeLists.txt, line 43 at r4 (raw file):

add_cppstyle(examples-template-example
	${CMAKE_CURRENT_SOURCE_DIR}/template-example/*.[ch])

You are saying cppstyle is an equivalent of cstyle?

examples/CMakeLists.txt, line 57 at r4 (raw file):

	add_executable(example-${name} ${srcs})
	target_include_directories(example-${name} PUBLIC ${LIBRPMA_INCLUDE_DIRS})
	target_link_libraries(example-${name} rpma ${LIBRPMA_LIBRARIES} ${LIBPMEM_LIBRARIES})

No libpmem here.

examples/CMakeLists.txt, line 67 at r4 (raw file):

	set(CURSES_NEED_NCURSES TRUE)
	find_package(Curses QUIET)
endif()

I guess we do not need it.

cmake_minimum_required(VERSION 3.3)
project(template-example CXX)

CXX niet.

examples/template-example/CMakeLists.txt, line 13 at r4 (raw file):

if(NOT WIN32)
	find_package(PkgConfig QUIET)
endif()

No Windows here.

examples/template-example/CMakeLists.txt, line 23 at r4 (raw file):

link_directories(${LIBRPMA_LIBRARY_DIRS})

add_executable(template-example template-example.cpp)

cpp

examples/template-example/README, line 1 at r4 (raw file):

Persistent Memory Development Kit

remove.

examples/template-example/README, line 3 at r4 (raw file):

Persistent Memory Development Kit

This is examples/librpma/README.

This is examples/template-example/README.

src/CMakeLists.txt, line 23 at r4 (raw file):

target_link_libraries(rpma PRIVATE
	${LIBPMEM_LIBRARIES}

no pmem here/

Previously, ldorau (Lukasz Dorau) wrote…

It is needed by the current POC implementation of RPMA

It was handy during POC. I guess we can get rid of for now. We will add later if it will be really needed.

tests/memcheck-stdcpp.supp, line 11 at r4 (raw file):

   fun:_dl_init
   obj:*/ld-*.so
}

No CPP here. You can remove.

tests/cmake/run_default.cmake, line 12 at r4 (raw file):

execute(${TEST_EXECUTABLE} ${DIR}/testfile)

finish()

It does not look right.
I'm not an expert but... in other projects this file is in tests/ directory as well as helpers.cmake.
I think paths here are messed up.

marcinslusarz

Reviewable status: 67 of 70 files reviewed, 62 unresolved discussions (waiting on @grom72, @janekmi, @ldorau, @lukaszstolarczuk, and @marcinslusarz)

utils/docker/images/Dockerfile.fedora-31, line 12 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

It was handy during POC. I guess we can get rid of for now. We will add later if it will be really needed.

queue.h in pmdk was needed because of Windows, are you going to support it in rpma?

Previously, janekmi (Jan Michalski) wrote…

@grom72 @marcinslusarz ?

definitely not me

ldorau

Reviewable status: 67 of 70 files reviewed, 62 unresolved discussions (waiting on @grom72, @janekmi, @ldorau, @lukaszstolarczuk, and @marcinslusarz)

.codecov.yml, line 7 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

A-Z order?

cmake?

Done.

Previously, janekmi (Jan Michalski) wrote…

?

???

Previously, janekmi (Jan Michalski) wrote…

set(VERSION_PATCH wip)
It is possible?

No, I added:
set(VERSION_PRERELEASE wip)

http://www.doxygen.nl/manual/config.html

Previously, janekmi (Jan Michalski) wrote…

My ubuntu has libibverbs 1.6. ;-)

Done.

CMakeLists.txt, line 31 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Not used?

Removed

CMakeLists.txt, line 32 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.?

Required by configure_package_config_file

CMakeLists.txt, line 33 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.?

Removed

CMakeLists.txt, line 34 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

.?

Required by check_c_compiler_flag

cmake/FindLIBIBVERBS.cmake, line 6 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Are you sure? My ubuntu responds properly:
$ pkg-config libibverbs --moversion
1.6.24.0

Indeed. Pkgconfig finds them now. Strange ... OK, I am removing them.

cmake/functions.cmake, line 13 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Not used.

Removed

cmake/functions.cmake, line 67 at r3 (raw file):

Previously, janekmi (Jan Michalski) wrote…

cpp

Renamed to add_cstyle

cmake/librpma.pc.in, line 8 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

👍

Requires:

libibverbs

librdmacm

Done.

cmake/packages.cmake, line 44 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

.

Done.

cmake/packages.cmake, line 50 at r2 (raw file):

Previously, marcinslusarz (Marcin Ślusarz) wrote…

.

Done.

doc/librpma.7.md, line 7 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

0.1 at best. ;-)

Done.

doc/librpma.Doxyfile.in, line 109 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Can you refer the doxy documentation on it? I do not know it is right either way.

"Each statement consists of a TAG_NAME written in capitals, followed by the equal sign (=) and one or more values. If the same tag is assigned more than once, the last assignment overwrites any earlier assignment. For tags that take a list as their argument, the += operator can be used instead of = to append new values to the list. Values are sequences of non-blanks. "

doc/librpma.Doxyfile.in, line 183 at r1 (raw file):

Previously, janekmi (Jan Michalski) wrote…

👍

We can test it later:

http://www.doxygen.nl/manual/config.html#cfg_generate_man

GENERATE_MAN
If the GENERATE_MAN tag is set to YES, doxygen will generate man pages for classes and files.

The default value is: NO.

doc/librpma.Doxyfile.in, line 1 at r2 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

Yes.

Done.

examples/CMakeLists.txt, line 7 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

No Windows here.

Done.

examples/CMakeLists.txt, line 37 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Includes should be stored only in the include directory.
Please remove src from here.

Done.

examples/CMakeLists.txt, line 43 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

You are saying cppstyle is an equivalent of cstyle?

Yes. I renamed it to cstyle

examples/CMakeLists.txt, line 57 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

No libpmem here.

Done.

examples/CMakeLists.txt, line 67 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I guess we do not need it.

Removed

examples/template-example/CMakeLists.txt, line 7 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

CXX niet.

Done.

examples/template-example/CMakeLists.txt, line 13 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

No Windows here.

Removed

examples/template-example/CMakeLists.txt, line 23 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

cpp

Done.

examples/template-example/README, line 1 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

remove.

Done.

examples/template-example/README, line 3 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

This is examples/template-example/README.

Done.

src/CMakeLists.txt, line 23 at r4 (raw file):

Previously, janekmi (Jan Michalski) wrote…

no pmem here/

Done.