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

Added travis.yml #4

Closed
wants to merge 1 commit into from
Closed

Added travis.yml #4

wants to merge 1 commit into from

Conversation

UnrealQuester
Copy link
Contributor

Your CONTRIBUTING.md states that you are looking for a third-party service to run your tests on. Travis is a continous integration service that builds every commit and even pull requests for you. You can log in using your github account and add your repository. Every time you push a new commit or someone submits a pull request, travis will build the commit and notify you if it failed. You can learn more about travis here.

This PR adds a .travis.yml so that travis knows how to run your tests. It currently tests clang 3.4 and g++ 4.8. For an example how this might look see https://travis-ci.org/UnrealQuester/better-enums/builds/68665505

@aantron
Copy link
Owner

aantron commented Jun 28, 2015

I had considered Travis and decided not to use it for now. The incremental benefit from testing on two Unix compilers (rather than one) is much less than from testing on both MSVC and a Unix compiler (rather than only one of those). This change may help people developing on MSVC the most, but there are more immediate challenges with MSVC, such as running cl.exe from the command line. So, this change could be nice for the time being, but I would ultimately prefer something more conclusive.

Perhaps Visual Studio Online can help with MSVC testing, and then we can use Travis for Unix testing. I spent some time looking into that setup, but ultimately wrote CONTRIBUTING.md instead :)

I don't have enough experience to know this: is it relatively easy to get Travis to test on more than one version each of clang and gcc?

@UnrealQuester
Copy link
Contributor Author

This should be trivial to accomplish. If you are fine with using travis I could write up a matrix for all compiler versions mentioned in test.py.

For windows there is appveyor, but I remember that being a pain to set up.

@aantron
Copy link
Owner

aantron commented Jun 28, 2015

I don't remember why I rejected AppVeyor before. The reason may since have become moot. I'm looking at it again now.

Getting automated builds on MSVC is a high priority compared to clang/gcc, so I think the best thing to do is for me to focus on setting up AppVeyor. If, after that, it looks straightforward to also build with clang and gcc in AppVeyor's Windows environment (as weird as that sounds), then I will opt for that. Otherwise, we should extend this travis.yml to build on more gcc and clang versions, and use both AppVeyor and Travis.

My reasoning is that a typical developer on a Unix-like system is probably using something like gcc 4.9 or clang 3.6 locally. In my experience, the behavior of recent versions of gcc and clang is mostly equivalent for the features enum.h uses, so running an automated test is unlikely to reveal anything – the exceptions to this are gcc before 4.7, so those are the most useful to include in travis.yml. On the other hand, testing the code on MSVC is quite likely to reveal problems. The converse doesn't hold. Code written to compile on MSVC is pretty likely to work on gcc/clang. In short, MSVC is the main problem compiler, followed by old gcc.

@UnrealQuester
Copy link
Contributor Author

While working on a version with all compilers installed I noticed that g++-3.4 is in that list. This compiler version is pretty old. So old in fact, that it is not even in any package repository available for ubuntu precise. As a result, I have been unable to install it. Would you mind if I removed this compiler version from test.py?

@aantron
Copy link
Owner

aantron commented Jun 28, 2015

I think you mean g++ 4.3. I prefer you not remove any of the compilers covered by test.py --all, but either I or you can add an option such as test.py --travis, or the like.

test.py --all is really only for me to run. It covers the compilers that I have access to and decided to test on, for the sake of thoroughness. I never intended it for general usage either by users or CI services. If I limit test.py --all to only what Travis/AppVeyor support, I give up decision making on supported compilers to Travis/AppVeyor/the Ubuntu maintainers, which I am not willing to do. Consider the following paragraph.

What it's looking like now is that CI services will cover a bunch of gcc and clang versions (Travis), and only some recent versions of MSVC (AppVeyor). If AppVeyor still supports even VS2013, they don't make it obvious. Since I want to cover more than what AppVeyor seems to support, I will need to keep test.py --all for my own use even after we set up CI. CI with AppVeyor and Travis will only be a useful approximation to whether a commit okay – the final say will be after I (or someone with too many compilers) is able to confirm with test.py --all.

I am testing out AppVeyor now, by the way.

@UnrealQuester
Copy link
Contributor Author

Will try to get a build matrix running on travis with as many compilers as I can.

Appveyor images do have VS2013 installed. I can confirm that Windows Server 2012 R2 does.

@aantron
Copy link
Owner

aantron commented Jun 28, 2015

Cool, thanks, and thanks!

@UnrealQuester
Copy link
Contributor Author

Quick update: The build matrix currently looks like this.
Just need to add some options to test.py for the older compiler versions.

@aantron
Copy link
Owner

aantron commented Jun 29, 2015

Thanks. I see that you are not using test.py's internal "matrix", which is the same conclusion I am coming to with AppVeyor. I think it's time to reengineer test.py slightly. I'm creating a simple "driver" module in Python that knows how to start each compiler that Better Enums positively claims to compile on, in each supported configuration for that compiler. That should make it easier to break up test.py --all into multiple independent calls to test.py, one per CI build matrix row. It should also help to run locally with the right options for whatever compiler the developer is using.

@UnrealQuester
Copy link
Contributor Author

This looks like a use case for CMake, actually. We can check if a compiler supports a specific flag with CHECK_CXX_COMPILER_FLAG and starting with 3.1 we can also check for specific features. This way we don't have do update the list for every compiler. There are also script out there for cmake < 3.1 that do this. The only problem would be testing multiple compilers in one go. We would have to write a script that calls cmake for every version.

For now, I could just add options to test.py for modern_gnu, gnu_re_constexpr and gnu_pre_enum_class and configure in travis what compiler needs what flag.

@aantron
Copy link
Owner

aantron commented Jun 30, 2015

I actually started on that at some point after I sent the last message. I uploaded the CMake branch, in the interest of coordination. It's still very basic. You can see the diff here: https://github.com/aantron/better-enums/compare/cmake

I'm a CMake newb, as in this is the first time I use it, so don't hesitate to give any advice if you have experience.

Also, I checked out options for building on multiple clang/gcc versions in AppVeyor yesterday, and decided that it's easier to just use Travis side-by-side.

@UnrealQuester
Copy link
Contributor Author

The current matrix looks like this. The clang builds fail because they have a test case which compiles with c++98. enum.h uses char32_t and char16_t if clang >= 2.9. But as these types are a c++11 extension and clang is compiling these with c++98 it (correctly) fails to build.

g++ only passes these tests because it also checks for GXX_EXPERIMENTAL_CXX0X and __cplusplus.

@UnrealQuester
Copy link
Contributor Author

Going to take a look at the CMake branch now.
Not sure how much of cxxtestgen we need for this. This might be straightforward with FindCxxTest.cmake.
Globbing files is potentially dangerous. CMake finds all files matching the glob and creates a solution/makefile/whatever for them. If the list of files change, cmake has to be re-run.
If you manually add the files, you just have to re-run make.
But using CMake is a good idea. We can generate native VS .sln files and run them.

@aantron
Copy link
Owner

aantron commented Jun 30, 2015

I am aware of the danger of globbing, but I am not aware of a good way to do this automatically. I'd rather not resort to generating CMakeLists.txt. That's a little ridiculous :)

As for the clang issue, the feature detection code is indeed apparently wrong. My local clang accepts char16_t and char32_t in C++98 mode, however. Quite interesting. Anyway, I am checking what __cplusplus is defined as in versions of clang where C++11 support was experimental, and then making a patch.

@aantron
Copy link
Owner

aantron commented Jun 30, 2015

Since I'm having trouble reproducing the problem with the compilers I already have installed, can you give this a try in Travis? https://github.com/aantron/better-enums/compare/clang-char-types

This check might miss some old clang versions that took the -std=c++0x flag, if they still defined __cplusplus as 199711, but I don't have any such compilers handy, and I am not currently aiming to support them either.

EDIT: Actually, that might be slightly wrong, since clang probably supported long long as an extension way before C++11. But I would still like to know if this compiles in Travis now.

@UnrealQuester
Copy link
Contributor Author

You can check the status of the new build with your fix here.

@aantron
Copy link
Owner

aantron commented Jun 30, 2015

Cool, watching it.

In the interest of me not wasting your time by us working on the same thing, I suggest we do this:

  • I get testing working with CMake and make sure it's suitable for local builds on my machines and in AppVeyor. This should be done by the end of today (my time), maybe tomorrow.
  • It should then be straightforward to adapt the testing scripts to Travis.

We will have one build matrix row per combination of compiler/configuration.

@UnrealQuester
Copy link
Contributor Author

Sounds good. We will see how big the build matrix gets for travis. While travis does not have a upper limit in total build time and size, the huge delay between single builds blows up the total build time. The current build was created on hour ago and only completed 4 out of 11 builds. We might want to have one row per compiler and iterate over all possible combinations in the build script.

Edit: this appears to be a problem on travis' side

@aantron
Copy link
Owner

aantron commented Jun 30, 2015

I am also reconsidering one row per combination for another reason, which is it might be a good idea to give developers the ability to test all configurations on their compiler locally. Perhaps it won't be fast enough to use while first implementing a change, but these tests can be run before submitting a pull request. Once we have this ability, it will be easy to trigger it in Travis/AppVeyor, and we can have one matrix row per compiler.

@aantron
Copy link
Owner

aantron commented Jul 2, 2015

Ok, the changes are in master now. You can see the AppVeyor build status here.

It should now be possible to test in Travis by running make COMPILER=[compiler] unix in directory test/. The time utility is no longer necessary, as timing tests are not done automatically. There is one build matrix row per compiler. The Makefile will try all configurations. Configurations not supported by a particular compiler are "vacuous" successes. CxxTest is still required.

@UnrealQuester
Copy link
Contributor Author

Rebased my work on top of master und currently running the tests.

@aantron
Copy link
Owner

aantron commented Jul 2, 2015

You may want to also try make default-all. I'm guessing CMake will detect the compiler you are installing. make COMPILER=c++ unix looks a little funny :) The point of that target is to select a compiler when the CMake default isn't the right choice.

Also, some upcoming nits:

  • Do you need ppa:dhart/ppa? It looks like you were using that to get CxxTest, but you are currently cloning it from GitHub.
  • I'm assuming you will eventually flatten these into one commit. For consistency, please add a period to the commit message.

After this goes in, I will commit an updated CONTRIBUTING.md and CONTRIBUTORS.

@aantron
Copy link
Owner

aantron commented Jul 2, 2015

I'm seeing this in the Travis testing output: /bin/sh: 6: 0: not found. Try incorporating this fix. If it works, I will commit it to master. I'm having a small amount of trouble reproducing the problem, since apparently my sh is a symlink for bash.

@UnrealQuester
Copy link
Contributor Author

Fix added.
PPA removed.
Changed the compiler to $COMPILER. I need to specify the compiler because the travis image already contains a c++ compiler and I need to make sure CMake picks our compiler.

@UnrealQuester
Copy link
Contributor Author

All tests passed for the latest build. Is there anything else you would like me to add?

@aantron
Copy link
Owner

aantron commented Jul 2, 2015

Looks good :) Ready for the squashed & rebased patch.

@UnrealQuester
Copy link
Contributor Author

@aantron done.

@aantron
Copy link
Owner

aantron commented Jul 2, 2015

In as 1769fbb. Thanks!

@aantron aantron closed this Jul 2, 2015
@UnrealQuester UnrealQuester deleted the travis branch July 3, 2015 14:43
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 this pull request may close these issues.

2 participants