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

Add build step for ICPC #3229

Closed
wants to merge 15 commits into from
Closed

Add build step for ICPC #3229

wants to merge 15 commits into from

Conversation

nlohmann
Copy link
Owner

This PR adds a CI build step for the Intel C++ Compiler

@nlohmann nlohmann marked this pull request as draft December 29, 2021 21:55
@coveralls
Copy link

coveralls commented Dec 30, 2021

Coverage Status

Coverage remained the same at 100.0% when pulling f9dfd63 on icpc into b21c345 on develop.

@nlohmann nlohmann added the state: help needed the issue needs help to proceed label Dec 30, 2021
@nlohmann
Copy link
Owner Author

ICPC fails to build the test suite.

/__w/json/json/test/src/unit-regression2.cpp(810): error: more than one instance of constructor "std::filesystem::__cxx11::path::path" matches the argument list:
            function "std::filesystem::__cxx11::path::path(std::filesystem::__cxx11::path &&) noexcept"
            function "std::filesystem::__cxx11::path::path(std::filesystem::__cxx11::path::string_type &&, std::filesystem::__cxx11::path::format)"
            argument types are: (json)
          CHECK_THROWS_WITH_AS(nlohmann::detail::std_fs::path(json(1)), "[json.exception.type_error.302] type must be string, but is number", json::type_error);
          ^
/usr/include/c++/9/bits/fs_path.h(473): note: this candidate was rejected because mismatch in count of arguments
      path(basic_string_view<value_type> __str, _Type __type)
      ^
/usr/include/c++/9/bits/fs_path.h(213): note: this candidate was rejected because mismatch in count of arguments
        path(_InputIterator __first, _InputIterator __last, const locale& __loc,
        ^
/usr/include/c++/9/bits/fs_path.h(205): note: this candidate was rejected because mismatch in count of arguments
        path(_Source const& __source, const locale& __loc, format = auto_format)
        ^
/usr/include/c++/9/bits/fs_path.h(198): note: this candidate was rejected because mismatch in count of arguments
        path(_InputIterator __first, _InputIterator __last, format = auto_format)
        ^
/usr/include/c++/9/bits/fs_path.h(191): note: this candidate was rejected because at least one template argument could not be deduced
        path(_Source const& __source, format = auto_format)
        ^
In file included from /__w/json/json/test/src/unit-algorithms.cpp(32):
/__w/json/json/single_include/nlohmann/json.hpp(3821): error: this operator is not allowed in a constant expression
  #elif JSON_HAS_FILESYSTEM
        ^

In file included from /__w/json/json/test/src/unit-algorithms.cpp(32):
/__w/json/json/single_include/nlohmann/json.hpp(4254): error: this operator is not allowed in a constant expression
  #if JSON_HAS_FILESYSTEM || JSON_HAS_EXPERIMENTAL_FILESYSTEM
      ^

In file included from /__w/json/json/test/src/unit-algorithms.cpp(32):
/__w/json/json/single_include/nlohmann/json.hpp(4507): error: this operator is not allowed in a constant expression
  #elif JSON_HAS_FILESYSTEM
        ^

In file included from /__w/json/json/test/src/unit-algorithms.cpp(32):
/__w/json/json/single_include/nlohmann/json.hpp(4887): error: this operator is not allowed in a constant expression
  #if JSON_HAS_FILESYSTEM || JSON_HAS_EXPERIMENTAL_FILESYSTEM
      ^

@nlohmann nlohmann mentioned this pull request Dec 30, 2021
@falbrechtskirchinger
Copy link
Contributor

Have a look at #3377. I had to disable the CHECK_THROWS_WITH_AS in #3379.

@nlohmann
Copy link
Owner Author

nlohmann commented Mar 8, 2022

Have a look at #3377. I had to disable the CHECK_THROWS_WITH_AS in #3379.

Thanks for pointing this out. I am now confused as well. If I would just comment out the test, I'm afraid that I would ignore something not working...

@falbrechtskirchinger
Copy link
Contributor

Adding $<$<CXX_COMPILER_ID:Intel>:-diag-disable=2196> or $<$<CXX_COMPILER_ID:Intel>:-wd2196> (deprecated) should get rid of

warning #2196: routine is both "inline" and "noinline"

if that is still an issue with current doctest.

- name: cmake
run: cmake -S . -B build -DJSON_CI=On
- name: build
run: >
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest using the literal style block scalar | over the flow style one > for multiline commands:

        run: |
          . /opt/intel/oneapi/setvars.sh
          cmake --build build --target ci_icpc

@@ -49,6 +49,9 @@ target_compile_options(test_main PUBLIC
# https://github.com/nlohmann/json/issues/1114
$<$<CXX_COMPILER_ID:MSVC>:/bigobj> $<$<BOOL:${MINGW}>:-Wa,-mbig-obj>

# https://github.com/nlohmann/json/pull/3229
$<$<CXX_COMPILER_ID:Intel>:-diag-disable=2196>
Copy link
Contributor

@falbrechtskirchinger falbrechtskirchinger May 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may need a version check. It's -wd2196 for older versions but I'm not familiar with ICPC, so I don't know since when the new flag is available. 🤷‍♂️

Also, 1786 to suppress deprecation warnings.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find a version number in the documentation.

COMMAND ${CMAKE_COMMAND}
-DCMAKE_BUILD_TYPE=Debug -GNinja
-DCMAKE_C_COMPILER=icc -DCMAKE_CXX_COMPILER=icpc
-DCMAKE_CXX_FLAGS="-DJSON_HAS_FILESYSTEM=0 -DJSON_HAS_EXPERIMENTAL_FILESYSTEM=0"
Copy link
Contributor

@falbrechtskirchinger falbrechtskirchinger May 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appears as -DJSON_HAS_FILESYSTEM=0\ -DJSON_HAS_EXPERIMENTAL_FILESYSTEM=0 on the commandline and results in:

/__w/json/json/single_include/nlohmann/json.hpp(4116): error: this operator is not allowed in a constant expression
  #elif JSON_HAS_FILESYSTEM

Remove? Don't know why <filesystem> detection was an issue on ICPC but might have been fixed by now?
Edit: Remove. Works (almost; see below) without.

@falbrechtskirchinger
Copy link
Contributor

falbrechtskirchinger commented May 1, 2022

Intel needs to be excluded here as well. :-(

#if JSON_HAS_FILESYSTEM || JSON_HAS_EXPERIMENTAL_FILESYSTEM
    // JSON_HAS_CPP_17 (do not remove; see note at top of file)
    SECTION("issue #3070 - Version 3.10.3 breaks backward-compatibility with 3.10.2 ")
    {
        nlohmann::detail::std_fs::path text_path("/tmp/text.txt");
        json j(text_path);

        const auto j_path = j.get<nlohmann::detail::std_fs::path>();
        CHECK(j_path == text_path);

#if !defined(_MSC_VER) && !(defined(__GNUC__) && __GNUC__ == 8 && __GNUC_MINOR__ < 4)
        // works everywhere but on MSVC and GCC <8.4
        CHECK_THROWS_WITH_AS(nlohmann::detail::std_fs::path(json(1)), "[json.exception.type_error.302] type must be string, but is number", json::type_error);
#endif
    }
#endif

Maybe this condition should just be changed to:

#if defined(__clang__) || ((defined(__GNUC__) && !defined(__INTEL_COMPILER)) && (__GNUC__ > 8 || (__GNUC__ == 8 && __GNUC_MINOR__ >= 4)))
        // only known to work on Clang and GCC >=8.4
        CHECK_THROWS_WITH_AS(nlohmann::detail::std_fs::path(json(1)), "[json.exception.type_error.302] type must be string, but is number", json::type_error);
#endif

Edit: We don't have JSON_HEDLEY_GNUC_VERSION_CHECK in the test suite. Fixed.
Edit 2: Condition was still not correct. Hopefully now.

@falbrechtskirchinger
Copy link
Contributor

falbrechtskirchinger commented May 1, 2022

Another issue:

    SECTION("issue #1647 - compile error when deserializing enum if both non-default from_json and non-member operator== exists for other type")
    {
#if !(defined(__INTEL_COMPILER) && __cplusplus >= 202000)
        {
            json j;
            NonDefaultFromJsonStruct x(j);
            NonDefaultFromJsonStruct y;
            CHECK(x == y);
        }
#endif

        auto val = nlohmann::json("one").get<for_1647>();
        CHECK(val == for_1647::one);
        json j = val;
    }

Test fails to compile on ICPC without the preprocessor guard when targeting C++20.

/__w/json/json/tests/src/unit-regression2.cpp(517): error: too many initializer values
              NonDefaultFromJsonStruct x(j);

These lines compile fine:

    auto x2 = j.get<NonDefaultFromJsonStruct>();
    auto x3 = j.operator NonDefaultFromJsonStruct();

Edit: First try #if condition didn't work.

@falbrechtskirchinger
Copy link
Contributor

@nlohmann I'm still waiting for CI, so I don't know if anything else is needed. In any event, there are now a quite a few changes. You can either reference my branch here or I can open a PR, whichever you prefer.

@nlohmann
Copy link
Owner Author

nlohmann commented May 1, 2022

@nlohmann I'm still waiting for CI, so I don't know if anything else is needed. In any event, there are now a quite a few changes. You can either reference my branch here or I can open a PR, whichever you prefer.

Please open a PR. You're already much deeper in this topic.

@falbrechtskirchinger
Copy link
Contributor

Okay. Of course CI just failed so there's at least one more issue to solve...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state: help needed the issue needs help to proceed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants