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

Extend CI workflow #13

Closed
friendlyanon opened this issue Oct 9, 2020 · 9 comments · Fixed by #14
Closed

Extend CI workflow #13

friendlyanon opened this issue Oct 9, 2020 · 9 comments · Fixed by #14

Comments

@friendlyanon
Copy link
Contributor

friendlyanon commented Oct 9, 2020

Right now the CI workflow only runs on Ubuntu and whatever default compiler it comes set up with.

I'd like to get some feedback on what are desirable compilers and what version(s) of them would you like to have running in CI?

Also, if the CI workflow will be extended to many OSs and compiler versions, I believe that only a single job should be dedicated to running tests with the default compiler, where the library is installed and imported using find_package.

@Kaaserne
Copy link
Owner

Kaaserne commented Oct 9, 2020

I'd like to get some feedback on what are desirable compilers and what version(s) of them would you like to have running in CI?

As for the desirable compilers, I'd like to see at least Clang, GCC and the MSVC compiler as well. As for its corresponding versions, I'd like to see an older one and a newer one. The old travis configuration compiled the tests with clang version 7.0.0 (tags/RELEASE_700/final) and gcc (Ubuntu 5.4.0-6ubuntu1~16.04.12) 5.4.0 20160609. I think it would be nice to compile the tests with even older compilers; one that at least supports C++11 and a configuration where the tests gets compiled with the most recent compilers, so we have some sort of "best of both worlds" situation. I think it would also be nice to see that C++11, 14, 17 and 20 can get tested.

Also, if the CI workflow will be extended to many OSs and compiler versions, I believe that only a single job should be dedicated to running tests with the default compiler, where the library is installed and imported using find_package.

I give you complete freedom in that, CMake and CI are not my strongest skills. I think you know better than me what's best practice for CMake and CI terrain.

@friendlyanon
Copy link
Contributor Author

friendlyanon commented Oct 9, 2020

Hmm, so the library itself has to be built with specific versions of compilers, but the tests aren't.
That means that in the regular cases the target compiler is used to build and install, and for the tests whatever else can be used. Something like so:

CXX=clang++-7 cmake -S . -B build/lib
cmake --build build/lib
cmake --install build/lib
cmake -S tests -B build/tests -D TEST_INSTALLED_VERSION:BOOL=YES # no CXX passed
cmake --build build/tests
cd build/tests && ctest

Since 2 compilers in one CMake run can't be used, the library must be installed.

@Kaaserne
Copy link
Owner

Kaaserne commented Oct 9, 2020

Hmm, so the library itself has to be built with specific versions of compilers, but the tests aren't.

Sorry, I meant the build and the tests must be made with GCC, Clang and MSVC. These three compilers must contain an old and a new(er) version.

The reason for that, by the way, is because in the past, I have had no problems building with the latest compiler version locally, but the older compiler versions sometimes suddenly gave a build error (in the former CI, that used Travis).

@friendlyanon
Copy link
Contributor Author

friendlyanon commented Oct 10, 2020

Well then, I have this run where GCC 4.8 seems to not be able to link and VS2017 is complaining about missing headers.

Furthermore, I have to inject the PIC flag into the {fmt} directory scope to get things to link properly. See this log for the error.

@Kaaserne
Copy link
Owner

Kaaserne commented Oct 10, 2020

Alright, could you make a PR for the CI tweaks you made so far? Then I can try to fix these issues. Also, during the time I was reproducing this error, compiling with \x86_64-w64-mingw32-gcc-4.8.0-win64_rubenvb, I get an error regarding ref-qualifiers for member functions:

C:/Users/marcd/CLionProjects/untitled/fmt/include/fmt/format.h:831:45: note: in expansion of macro 'FMT_NOEXCEPT'
   uint128_wrapper& operator+=(uint64_t n) & FMT_NOEXCEPT {
                                             ^

I think this is an error on {fmt} side. When removing the ref qualifier, the GCC compiler says it doesn't recognize the -mbig-obj flag too. While {fmt} says this on their overview docs site:


Portability

The library is highly portable and relies only on a small set of C++11 features:

  • variadic templates
  • type traits
  • rvalue references
  • decltype
  • trailing return types
  • deleted functions
  • alias templates

These are available since GCC 4.8, Clang 3.0 and MSVC 19.0 (2015). For older compilers use {fmt} version 4.x which continues to be maintained and only requires C++98.


This is all so very confusing.

PS:

Just found out that:

https://akrzemi1.wordpress.com/2014/06/02/ref-qualifiers/


Ref-qualifiers in your compiler

Does your compiler support ref-qualifiers? The first proposal for rvalue references was submitted on 2002 (N1377), whereas the first proposal for member function ref-qualifiers was submitted 3 years later, on 2005 (N1784). This lag is also reflected in compiler support of these features. The following table summarizes the state as I know it:

Compiler rvalue refs ref-qualifiers source
GCC 4.3 4.8.1 here
Clang 2.9 2.9 here
Intel 12.0 14.0 here
Visual C++ VC10 Nov 2013 CTP here

I think I'll need GCC 4.8.1

PSPS:

I've created an issue regarding this aswell on the library {fmt}

@friendlyanon
Copy link
Contributor Author

friendlyanon commented Oct 10, 2020

Any idea what the Position Independent Code requirement could be about?
It seems weird that the -fPIC flag has to be injected to link against fmt.a

Note: you might want to create a develop branch to develop normally and pull changes over to master as to not run CI all the time.
The workflow_dispatch event also enables you to manually run the CI workflow on any branch.

@Kaaserne Kaaserne reopened this Oct 11, 2020
@Kaaserne
Copy link
Owner

Kaaserne commented Oct 11, 2020

Whoops, closed PR per accident. I've read that this -fPIC flag is for CPU's without MMU, which doesn't make sense, or for making shared libraries but that also doesn't make sense. I've seen this issue and this issue that may help. I'll try and do some more research on this, otherwise I'll just ask the guys of fmt on the same issue I've already created regarding the mbig-obj flag.

I'll create a development branch, good point. https://github.com/MarcDirven/cpp-lazy/tree/dev

@friendlyanon
Copy link
Contributor Author

The issue closing was intentional with the inclusion of specific terms in the PR description, eg fixes#nn. This issue was opened for feedback about the extensions of the CI workflow, which I think we seccessfully discussed.
There is no problem with keeping it closed but continuing related discussion.

Regarding -fPIC, since {fmt} does not define library type, that add_library call is affected by BUILD_SHARED_LIBS's value, which by default is not defined, hence false, ie {fmt} is built as a static library as evident by the fmt.a artifact name.
Your library is explicitly defined as INTERFACE (usage requirements only) and for the tests it's consumed directly using an executable target.
Indeed, this doesn't make much sense.

@Kaaserne
Copy link
Owner

The issue closing was intentional with the inclusion of specific terms in the PR description, eg fixes#nn. This issue was opened for feedback about the extensions of the CI workflow, which I think we seccessfully discussed.
There is no problem with keeping it closed but continuing related discussion.

Right, I'll close this for now. I've also asked {fmt} what's up with the -fPIC flag.

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

Successfully merging a pull request may close this issue.

2 participants