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

3.8.0: unit-cbor.cpp test failures #2189

Closed
2 of 5 tasks
dvzrv opened this issue Jun 14, 2020 · 26 comments · Fixed by #2202
Closed
2 of 5 tasks

3.8.0: unit-cbor.cpp test failures #2189

dvzrv opened this issue Jun 14, 2020 · 26 comments · Fixed by #2202
Assignees
Labels
Milestone

Comments

@dvzrv
Copy link

dvzrv commented Jun 14, 2020

Hi! I package nlohmann-json for Arch Linux. Upon building version 3.8.0 I ran the unit tests, but some fail.

For the last stable release (3.7.3) I used the below described workflow and the unit tests pass. Maybe something has changed in the meantime?

What is the issue you have?

Executing test-algorithms...
[doctest] doctest version is "2.3.7"
[doctest] run with "--help" for options
===============================================================================
[doctest] test cases:      1 |      1 passed |      0 failed |      0 skipped
[doctest] assertions:     39 |     39 passed |      0 failed |
[doctest] Status: SUCCESS!
Executing test-allocator...
[doctest] doctest version is "2.3.7"
[doctest] run with "--help" for options
===============================================================================
[doctest] test cases:      3 |      3 passed |      0 failed |      0 skipped
[doctest] assertions:     17 |     17 passed |      0 failed |
[doctest] Status: SUCCESS!
Executing test-alt-string...
[doctest] doctest version is "2.3.7"
[doctest] run with "--help" for options
===============================================================================
[doctest] test cases:      1 |      1 passed |      0 failed |      0 skipped
[doctest] assertions:     28 |     28 passed |      0 failed |
[doctest] Status: SUCCESS!
Executing test-bson...
[doctest] doctest version is "2.3.7"
[doctest] run with "--help" for options
===============================================================================
[doctest] test cases:      6 |      6 passed |      0 failed |      1 skipped
[doctest] assertions:    486 |    486 passed |      0 failed |
[doctest] Status: SUCCESS!
Executing test-capacity...
[doctest] doctest version is "2.3.7"
[doctest] run with "--help" for options
===============================================================================
[doctest] test cases:      1 |      1 passed |      0 failed |      0 skipped
[doctest] assertions:    120 |    120 passed |      0 failed |
[doctest] Status: SUCCESS!
Executing test-cbor...
[doctest] doctest version is "2.3.7"
[doctest] run with "--help" for options
===============================================================================
src/unit-cbor.cpp:1912:
TEST CASE:  single CBOR roundtrip
  sample.json

src/unit-cbor.cpp:1912: ERROR: test case THREW exception: exception thrown in subcase - will translate later when the whole test case has been exited (cannot translate while there is an active exception)

===============================================================================
src/unit-cbor.cpp:1912:
TEST CASE:  single CBOR roundtrip

src/unit-cbor.cpp:1912: ERROR: test case THREW exception: [json.exception.parse_error.101] parse error at line 1, column 1: syntax error while parsing value - unexpected end of input; expected '[', '{', or a literal

===============================================================================
src/unit-cbor.cpp:2350:
TEST CASE:  examples from RFC 7049 Appendix A
  byte arrays

src/unit-cbor.cpp:2500: ERROR: CHECK_NOTHROW( j = json::from_cbor(packed) ) THREW exception: "[json.exception.parse_error.110] parse error at byte 1: syntax error while parsing CBOR value: unexpected end of input"

src/unit-cbor.cpp:2505: ERROR: CHECK( j == json::binary(expected) ) is NOT correct!
  values: CHECK( null == {"bytes":[],"subtype":null} )

===============================================================================
[doctest] test cases:      5 |      3 passed |      2 failed |      1 skipped
[doctest] assertions: 1640832 | 1640830 passed |      2 failed |
[doctest] Status: FAILURE!

Full build log:
nlohmann-json-3.8.0-build.log

Please describe the steps to reproduce the issue.

I'm cloning this repository from the commit (as no annotated tags exist) and verify the lightweight tag/commit using the author's PGP signature.

I'm building with:

  cmake -DCMAKE_INSTALL_PREFIX='/usr' \
        -DCMAKE_INSTALL_LIBDIR='/usr/lib' \
        -DCMAKE_BUILD_TYPE='None' \
        -DBUILD_TESTING=ON \
        -DJSON_MultipleHeaders=ON \
        -Wno-dev \
        -B build \
        -S .
  make VERBOSE=1 -C build

Additional to the above I run the following to get pkgconfig integration (but that has nothing to do with the tests):

  meson --prefix=/usr \
        --libdir=lib \
        meson-build
  ninja -C meson-build

I run tests with:

make -k check

Note:
When running the test suite using ctest (e.g. as in use in the travis integration)

json/.travis.yml

Lines 324 to 328 in e7452d8

# compile and execute unit tests
- mkdir -p build && cd build
- cmake .. ${CMAKE_OPTIONS} -DJSON_MultipleHeaders=${MULTIPLE_HEADERS} -DJSON_BuildTests=On -GNinja && cmake --build . --config Release
- ctest -C Release --timeout 2700 -V -j
- cd ..

No tests are collected:

UpdateCTestConfiguration  from :/build/nlohmann-json/src/nlohmann-json-3.8.0/DartConfiguration.tcl
UpdateCTestConfiguration  from :/build/nlohmann-json/src/nlohmann-json-3.8.0/DartConfiguration.tcl
Test project /build/nlohmann-json/src/nlohmann-json-3.8.0
Constructing a list of tests
Updating test list for fixtures
Added 0 tests to meet fixture requirements
Checking test dependency graph...
Checking test dependency graph end
No tests were found!!!

Can you provide a small but working code example?

n/a

What is the expected behavior?

All unit-tests pass

And what is the actual behavior instead?

Some unit-cbor unit tests fail.

Which compiler and operating system are you using?

  • Compiler: gcc 10.1.0
  • Operating system: Arch Linux

Which version of the library did you use?

  • latest release version 3.7.3
  • other release - please state the version: 3.8.0
  • the develop branch

If you experience a compilation error: can you compile and run the unit tests?

  • yes
  • no - please copy/paste the error message below

see above

@nlohmann
Copy link
Owner

make check is no longer supported. I need to update the Makefile accordingly to avoid such confusion.

The test suite is now in a separate repository and is downloaded as part of ctest.

Can you please try to execute the unit tests as described in https://github.com/nlohmann/json#execute-unit-tests:

mkdir build
cd build
cmake .. -DJSON_BuildTests=On
cmake --build .
ctest --output-on-failure

@nlohmann nlohmann added solution: proposed fix a fix for the issue has been proposed and waits for confirmation and removed kind: bug labels Jun 14, 2020
@nlohmann nlohmann self-assigned this Jun 14, 2020
@dvzrv
Copy link
Author

dvzrv commented Jun 14, 2020

@nlohmann Thanks for getting back on this so quickly.

I see. Hm, I think downloading during build/test is not possible though. Can I download the tarball of the tests myself and extract it to a directory where it will be picked up?
This way I can have validated sources upon download :)

@nlohmann
Copy link
Owner

The code that does the download is here: https://github.com/nlohmann/json/blob/develop/cmake/download_test_data.cmake#L7, so this is possible:

mkdir build
cd build
cmake ..
make download_test_data

The test files are then downloaded to json_test_data inside build.

@dvzrv
Copy link
Author

dvzrv commented Jun 14, 2020

Okay, I can get it to work by downloading the required test data and then:

  mkdir -vp build
  mv -v json_test_data-2.0.0 build/json_test_data  
  cmake -DCMAKE_INSTALL_PREFIX='/usr' \
        -DCMAKE_INSTALL_LIBDIR='/usr/lib' \
        -DCMAKE_BUILD_TYPE='None' \
        -DBUILD_TESTING=ON \
        -DJSON_MultipleHeaders=ON \
        -Wno-dev \
        -B build \
        -S .
  make VERBOSE=1 -C build
  cd build
  ctest  --output-on-failure

I think it would be really really awesome to have a cmake option to point at a directory with the test data. For packagers this would be the preferred way, because it allows for properly doing checksums on tarballs for the test data, extract it and run the setup.
Basically making this here more flexible:

add_custom_target(download_test_data
COMMAND test -d json_test_data || ${GIT_EXECUTABLE} clone -c advice.detachedHead=false --branch v${JSON_TEST_DATA_VERSION} ${JSON_TEST_DATA_URL}.git --quiet --depth 1
COMMENT "Downloading test data from ${JSON_TEST_DATA_URL} (v${JSON_TEST_DATA_VERSION})"
WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
)
# create a header with the path to the downloaded test data
file(WRITE ${CMAKE_BINARY_DIR}/include/test_data.hpp "#define TEST_DATA_DIRECTORY \"${CMAKE_BINARY_DIR}/json_test_data\"\n")

@nlohmann
Copy link
Owner

Sorry, I do not understand your use case. Why do you want to checksum the test data? Why isn't the current approach sufficient?

@dvzrv
Copy link
Author

dvzrv commented Jun 15, 2020

Because this would download assets during build time.

On the background:
Packaging in Arch's case is separated into a preparation, build, test and package step. The cmake and make calls traditionally happen during build, the ctest call during the test step.
All assets required to build a piece of software should be locked by either commit/tag + signature or tarball + checksum and to be known prior to prepare/build/test/package to ensure a reproducible build.
"Exceptions" to this rule are packages relying on language specific package management (e.g. nodejs, golang or rust, which we link statically, but which lock their own dependencies by either commit hash or version). Partially this is of course also because packaging e.g. each one-liner nodejs library requires many many developers.

We have a policy (and this is quite similar to that of other distributions which even enforce this by disabling network services in their build infrastructure during build) to not download assets in the build or test steps (and certainly not in the package step of course), as these are files that we can not account for and potentially break a reproducible build.
Distributions such as Debian (with many more developers than Arch has) go to great lengths to patch these kind of scenarios and for many upstreams even have an entire copy of the source code just in case or have started packaging many hundreds of language-specific source packages (e.g. golang, java, nodejs) to build packages with packaged sources, etc.

@nlohmann nlohmann added confirmed and removed solution: proposed fix a fix for the issue has been proposed and waits for confirmation labels Jun 15, 2020
@nlohmann
Copy link
Owner

I understand. I shall have a look.

@xvitaly
Copy link
Contributor

xvitaly commented Jun 15, 2020

Fedora maintainer here. I cannot use Internet access during official builds. Please add solution to use pre-downloaded test data with Cmake option for example -DTEST_DATA_DIR=foo.

@nlohmann
Copy link
Owner

@xvitaly Is the approach described in #2189 (comment) feasible for you?

@xvitaly
Copy link
Contributor

xvitaly commented Jun 15, 2020

Is the approach described in #2189 (comment) feasible for you?

I will try it now.

@xvitaly
Copy link
Contributor

xvitaly commented Jun 15, 2020

@nlohmann Most of tests passed, but these were failed:

The following tests FAILED:
         50 - cmake_fetch_content_configure (Failed)
         51 - cmake_fetch_content_build (Not Run)
Errors while running CTest

@xvitaly
Copy link
Contributor

xvitaly commented Jun 15, 2020

@nlohmann Disabled broken tests. Added a temporary workaround to our package.

@nlohmann
Copy link
Owner

@xvitaly Thanks for reporting. Can you provide the output of the failed tests? Which CMake version are you using?

@nlohmann
Copy link
Owner

Would f4c4bab fix your issue?

@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label Jun 16, 2020
@nlohmann nlohmann added this to the Release 3.8.1 milestone Jun 16, 2020
@xvitaly
Copy link
Contributor

xvitaly commented Jun 16, 2020

Would f4c4bab fix your issue?

It works, but still require Git due to find_package(Git). Please move it under else() too.

cmake_fetch_content_configure and cmake_fetch_content_build are still broken.

@xvitaly
Copy link
Contributor

xvitaly commented Jun 16, 2020

Can you provide the output of the failed tests?

50/51 Test #50: cmake_fetch_content_configure ......***Failed    0.36 sec
-- The CXX compiler identification is GNU 10.1.1
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ - works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
[1/9] Creating directories for 'json-populate'
[1/9] Performing download step (git clone) for 'json-populate'
fatal: repository '/builddir/build/BUILD/json-3.8.0/test/cmake_fetch_content/project/../../..' does not exist
fatal: repository '/builddir/build/BUILD/json-3.8.0/test/cmake_fetch_content/project/../../..' does not exist
fatal: repository '/builddir/build/BUILD/json-3.8.0/test/cmake_fetch_content/project/../../..' does not exist
-- Had to git clone more than once:
          3 times.
CMake Error at json-subbuild/json-populate-prefix/tmp/json-populate-gitclone.cmake:31 (message):
  Failed to clone repository:
  '/builddir/build/BUILD/json-3.8.0/test/cmake_fetch_content/project/../../..'


FAILED: json-populate-prefix/src/json-populate-stamp/json-populate-download 
cd /builddir/build/BUILD/json-3.8.0/x86_64-redhat-linux-gnu/test/cmake_fetch_content/_deps && /usr/bin/cmake -P /builddir/build/BUILD/json-3.8.0/x86_64-redhat-linux-gnu/test/cmake_fetch_content/_deps/json-subbuild/json-populate-prefix/tmp/json-populate-gitclone.cmake && /usr/bin/cmake -E touch /builddir/build/BUILD/json-3.8.0/x86_64-redhat-linux-gnu/test/cmake_fetch_content/_deps/json-subbuild/json-populate-prefix/src/json-populate-stamp/json-populate-download
ninja: build stopped: subcommand failed.

CMake Error at /usr/share/cmake/Modules/FetchContent.cmake:912 (message):
  Build step for json failed: 1
Call Stack (most recent call first):
  /usr/share/cmake/Modules/FetchContent.cmake:1003 (__FetchContent_directPopulate)
  CMakeLists.txt:13 (FetchContent_Populate)


-- Configuring incomplete, errors occurred!
See also "/builddir/build/BUILD/json-3.8.0/x86_64-redhat-linux-gnu/test/cmake_fetch_content/CMakeFiles/CMakeOutput.log".

      Start 51: cmake_fetch_content_build
Failed test dependencies: cmake_fetch_content_configure
51/51 Test #51: cmake_fetch_content_build ..........***Not Run   0.00 sec

96% tests passed, 2 tests failed out of 51

Which CMake version are you using?

cmake-3.17.3-1.fc32.x86_64

@nlohmann
Copy link
Owner

4d96f4c moves the find_package(Git) and tries to fix the cmake_fetch_content_configure test. Please check.

@xvitaly
Copy link
Contributor

xvitaly commented Jun 16, 2020

moves the find_package(Git) and tries to fix the cmake_fetch_content_configure test. Please check.

Now it builds fine without installed Git. Thanks.

But tests cmake_fetch_content_configure and cmake_fetch_content_build are still broken due to missing Git and absent Internet access. New logs:

50/51 Test #50: cmake_fetch_content_configure ......***Failed    0.34 sec
-- The CXX compiler identification is GNU 10.1.1
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ - works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
CMake Error at /usr/share/cmake/Modules/ExternalProject.cmake:2454 (message):
  error: could not find git for clone of json-populate
Call Stack (most recent call first):
  /usr/share/cmake/Modules/ExternalProject.cmake:3271 (_ep_add_download_command)
  CMakeLists.txt:13 (ExternalProject_Add)


-- Configuring incomplete, errors occurred!
See also "/builddir/build/BUILD/json-3.8.0/x86_64-redhat-linux-gnu/test/cmake_fetch_content/_deps/json-subbuild/CMakeFiles/CMakeOutput.log".

CMake Error at /usr/share/cmake/Modules/FetchContent.cmake:900 (message):
  CMake step for json failed: 1
Call Stack (most recent call first):
  /usr/share/cmake/Modules/FetchContent.cmake:1003 (__FetchContent_directPopulate)
  CMakeLists.txt:12 (FetchContent_Populate)


-- Configuring incomplete, errors occurred!
See also "/builddir/build/BUILD/json-3.8.0/x86_64-redhat-linux-gnu/test/cmake_fetch_content/CMakeFiles/CMakeOutput.log".

      Start 51: cmake_fetch_content_build
Failed test dependencies: cmake_fetch_content_configure
51/51 Test #51: cmake_fetch_content_build ..........***Not Run   0.00 sec

96% tests passed, 2 tests failed out of 51

@nlohmann Just disable these tests if the JSON_TestDataDirectory variable is set.

@nlohmann
Copy link
Owner

I don't like the implicit dependency between "here is a directory of tests" and "don't execute this specific CMake test". I added a label git_required to the tests that would require a git checkout, see e86b3fa. You can skip these with

ctest -LE git_required

Please let me know if this is feasible for you.

@xvitaly
Copy link
Contributor

xvitaly commented Jun 17, 2020

Please let me know if this is feasible for you.

The problem is absent Internet access, not Git at all. I can install it in buildroot, but it will not download anything from the Internet.

@nlohmann
Copy link
Owner

I do think the problem is not related to missing Internet access. The cmake_fetch_content_configure does not download a remote Git repository, but assumes the root directory of the package is part of a Git checkout. I can reproduce the problem when downloading a ZIP from GitHub which does not contain a .git directory. Then cmake_fetch_content_configure fails complaining that

fatal: repository '/Users/niels/Downloads/json-develop/test/cmake_fetch_content/project/../../..' does not exist

@nlohmann
Copy link
Owner

@xvitaly Did you test the proposal of #2189 (comment)?

@xvitaly
Copy link
Contributor

xvitaly commented Jun 19, 2020

Did you test the proposal of #2189 (comment)?

Yes, it works great. Thank you.

100% tests passed, 0 tests failed out of 49

@nlohmann
Copy link
Owner

@dvzrv Is the approach in #2189 (comment) also working for you? If so, I would merge this.

@dvzrv
Copy link
Author

dvzrv commented Jun 20, 2020

@nlohmann I'm using a git clone of this repository, based on a tag which is checked against your PGP signature (but a tarball of the test repository) to build.

With that setup I get:

100% tests passed, 0 tests failed out of 52

Label Time Summary:
all             = 262.70 sec*proc (43 tests)
git_required    =   3.80 sec*proc (2 tests)

Running the tests against a tarball of this repository I get:

96% tests passed, 2 tests failed out of 52

Label Time Summary:
all             = 262.42 sec*proc (43 tests)
git_required    =   0.60 sec*proc (2 tests)

With applying the ctest switch:

100% tests passed, 0 tests failed out of 50

Label Time Summary:
all    = 265.20 sec*proc (43 tests)

Just out of curiosity: What is git used for in that context? Why is it relevant for the tests, if they are already downloaded and in place?

@nlohmann
Copy link
Owner

@dvzrv Good to know!

About your question: The test cmake_fetch_content_configure checks if it's possible to call CMake's FetchContent with the current repository state. For this, we pass the root directory of the library as GIT_REPOSITORY parameter to FetchContent_Declare in test/cmake_fetch_content/project/CMakeLists.txt. This does not work if the library was not checked out via Git. This test is for the maintainers and not really the user of the library.

Given that everything works for you, I will merge #2202.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@nlohmann @dvzrv @xvitaly and others