Conversation
| @@ -0,0 +1,2 @@ | |||
| Building a cxx library should run ocamlmklib with "-lstdc++". | |||
| $ dune build | |||
There was a problem hiding this comment.
How should I check that this test succeeds?
There was a problem hiding this comment.
If it fails a bunch of undefined symbols such as std::__1::cout errors are printed by the linker.
I will make that clear in the test description. Also the flag is not always -stdc++, it is -lc++ for clang.
Also there is no specific link flag for c++ compilation with MSVC so this test won't fail on windows.
There was a problem hiding this comment.
Ack. I'm not sure we run the testsuite on Windows BTW.
There was a problem hiding this comment.
Just one last thing: I was looking at the commits individually. I was expecting to see the test fail at first and then observe subsequent commits fix it, but it seems that it was succeeding from the start. Is that the case?
There was a problem hiding this comment.
Yes they are indeed failing but the error message was not promoted, do you want me to edit it ?
There was a problem hiding this comment.
That's alright, I just wanted to check it was the case. For next time, please do promote it. It's good practice to first demonstrate the problem and then demonstrate that it is fixed. Plus it's good if the testsuite passes for each commit.
6d3cb63 to
064f842
Compare
Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
This help ensure the path is correct at is is expected to be the build path. Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
and remove commented rules Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
Signed-off-by: Ulysse Gérard <thevoodoos@gmail.com>
064f842 to
d59567e
Compare
Default C++ flags require compiler detection and need the path to the build directory for that.
Previously the wrong path was sometime given. This PR fixes the issue and changed the signature of the function to require the context instead of the build path.
I discovered this problem when rebasing #4846
CC @recoules can you confirm that it fixes #4847 ? And maybe test it in a more realist setting ?