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

Build issue on certain MSVC in Cantera. #724

Closed
1uc opened this issue Apr 3, 2023 · 12 comments
Closed

Build issue on certain MSVC in Cantera. #724

1uc opened this issue Apr 3, 2023 · 12 comments

Comments

@1uc
Copy link
Collaborator

1uc commented Apr 3, 2023

The CI of https://github.com/Cantera uncovered a build issue for certain versions of MSVC. Some of the discussion started in #713.

The link to the failing CI is:
https://github.com/Cantera/cantera/actions/runs/4588940048/jobs/8103492434

It reports a

c:\miniconda3\envs\test\library\include\highfive\bits/H5Path_traits_misc.hpp(31): error C2248: 'HighFive::Object::Object': cannot access protected member declared in class 'HighFive::Object'
...
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.16.27023\include\filesystem(2392): note: see reference to class template instantiation 'std::chrono::time_point<std::filesystem::_File_time_clock,std::filesystem::_File_time_clock::duration>' being compiled
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.16.27023\include\type_traits(616): note: see reference to class template instantiation 'std::basic_string_view<wchar_t,std::char_traits<wchar_t>>' being compiled
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.16.27023\include\xstring(2124): note: see reference to class template instantiation 'std::is_convertible<const _StringViewIsh &,std::basic_string_view<wchar_t,std::char_traits<wchar_t>>>' being compiled

Which smell like SFINAE to check if something HighFive related can be converted to a string. While doing so it fails because the ctor of HighFive::Object is protected. To be investigated further.

@ischoegl
Copy link
Contributor

ischoegl commented Apr 3, 2023

Thanks, @1uc ... trying to debug a couple of other quirks, I updated/fixed our build scripts (unfortunately we're using a different build system - SCons rather than cmake) and I now can follow installation details a little better. The offending issue can now be bypassed, as our installation now falls back to an earlier git submodule 2.6.2.

The latest runner we're using does have a system installation of 2.7.0 from conda, which produces the following output for MSVC 14.1, which references line numbers.

c:\miniconda3\envs\test\library\include\highfive\bits/H5Path_traits_misc.hpp(31): error C2248: 'HighFive::Object::Object': cannot access protected member declared in class 'HighFive::Object'
c:\miniconda3\envs\test\library\include\highfive\bits/H5Object_misc.hpp(27): note: see declaration of 'HighFive::Object::Object'
c:\miniconda3\envs\test\library\include\highfive\H5Object.hpp(53): note: see declaration of 'HighFive::Object'
c:\miniconda3\envs\test\library\include\highfive\bits/H5Path_traits_misc.hpp(19): note: while compiling class template member function 'HighFive::PathTraits<HighFive::Attribute>::PathTraits(void)'
c:\miniconda3\envs\test\library\include\highfive\bits\../H5Attribute.hpp(124): note: see reference to function template instantiation 'HighFive::PathTraits<HighFive::Attribute>::PathTraits(void)' being compiled
c:\miniconda3\envs\test\library\include\highfive\bits\../H5Attribute.hpp(45): note: see reference to class template instantiation 'HighFive::PathTraits<HighFive::Attribute>' being compiled 

Update: after tweaking the CI configuration, you can see an example for a failing CI here (different from before, it now fails at the configuration step, which is by design):
https://github.com/Cantera/cantera/actions/runs/4600576598/jobs/8127358231?pr=1471

PS: Some of my other issues go back to #726, which created unexpected behavior as I was testing with both git submodule and a locally installed conda package. On the positive side, after fixing the relative import, I do not see any additional HighFive-related failures on Cantera on any of our build systems other than the one for MSVC 14.1 (which is the oldest version we're able to test).

@ischoegl
Copy link
Contributor

ischoegl commented Apr 4, 2023

@1uc ... tried a CI run compiling with what is slated for 2.7.1 thus far, and am still getting the same failure as the one on top. Unfortunately, there's no additional output in the error stack.

@1uc
Copy link
Collaborator Author

1uc commented Apr 4, 2023

I have a suspicion why this is. We (in order to appease a compiler) removed some friend declarations in this commit:
20be06f

Essentially, we're avoiding a few of these

class Foo {
    // ...
    template <typename Derivate>
    friend class PathTraits;
};

We should try to put them back conditionally for MSVC only.

@ischoegl
Copy link
Contributor

ischoegl commented Apr 4, 2023

Newer MSVC versions (2019 and later, i.e. 14.2 and 14.3) work as far as I can tell. MSVC toolset 14.1 is the one with issues on our end, but I'm not able to test older versions.

This was referenced Apr 4, 2023
@1uc
Copy link
Collaborator Author

1uc commented Apr 4, 2023

@ischoegl It seems to be this compiler bug:
https://developercommunity.visualstudio.com/t/MSVC-compiler-improperly-implements-N489/1516410

If we look at HighFive::File we find:

class File: public Object, public NodeTraits<File>, public AnnotateTraits<File> {
  // ... 

  protected:
    File() = default;

  private:
    using Object::Object;
};

The error doesn't manifest with --std=c++14 but only with --std=c++17 (just like in the description of the bug). We're using VS 2022 17.0; the bug report doesn't specify the compiler version.

We can get CI to fail by forcing --std=c++17:
https://github.com/BlueBrain/HighFive/actions/runs/4611634047/jobs/8151560905?pr=728#step:4:130

@ischoegl
Copy link
Contributor

ischoegl commented Apr 4, 2023

Confirming that we’re using the c++17 flag also; I’m happy that you can reproduce outside of our CI. Based on this being a compiler bug that’s unlikely to be fixed (under investigation since ‘21) putting in a dedicated flag is a tough call. Since HighFive is an optional dependency for us at the moment (we just switched over from h5py and only use it in the development version), it’s easy to address on our end.

PS: just noticed #728

@1uc
Copy link
Collaborator Author

1uc commented Apr 4, 2023

I think you beat me to it, but yes, we can make the issue go away:
https://github.com/BlueBrain/HighFive/actions/runs/4611730629/jobs/8151778661?pr=728

I'll discuss with the other developers to see if we want to keep the workaround.

@ischoegl
Copy link
Contributor

ischoegl commented Apr 4, 2023

👍 ... fwiw, I guess it's a function on how many people will try to use MSVC toolset 14.1 together with the c++17 option. We use it as it is presumably the oldest version that should fully support the standard (I believe after revision 14.14). That said, we don't need to include the newest HighFive version for tests in this combination.

As an aside, it would be interesting to see whether a GH runs-on: windows-2019 Action will produce the same behavior, apart from the open question whether the next VS will fix it. Looking over #727 / #728, restricting the workaround to the buggy MSVC toolset and not exposing HIGHFIVE_HAS_FRIENDS as a cmake flag may be an option. Regardless of the team’s decision, I'm truly impressed by the project - while I spent some time debugging, your responsiveness and support are truly appreciated!

@1uc
Copy link
Collaborator Author

1uc commented Apr 5, 2023

We've merged #728 into master. I tend to regret hard-coding anything build-system related; and since nobody is expected to set the variable, it acts pure as a safety valve that allows users to pick. Hopefully, it wont be used.

The name of the macro changed to HIGHFIVE_HAS_FRIEND_DECLARATIONS (or similar).

@1uc
Copy link
Collaborator Author

1uc commented Apr 6, 2023

@ischoegl Version 2.7.1 has been created. I'll close the issue since, most likely, it's been fixed. Please feel free to re-open or open a different issue as needed.

@1uc 1uc closed this as completed Apr 6, 2023
@ischoegl
Copy link
Contributor

ischoegl commented Apr 6, 2023

Thanks! While I'm relatively certain that all issues are resolved, I'll have to wait for the 2.7.1 conda packages to have final confirmation. Again, the HighFive team's efforts are truly appreciated!

@ischoegl
Copy link
Contributor

@1uc ... I am confirming that conda packages for 2.7.1 resolve everything on our side. Again, thanks for your assistance!

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

No branches or pull requests

2 participants