Skip to content

Conversation

@konserw
Copy link
Contributor

@konserw konserw commented Jan 10, 2016

Please consider compilation of own gmock and gtest using externalproject feature of CMake. I used version gmock 1.7 for this purpose. In my opinion it will be less troublesome. It may also help your efforts to setup windows CI.
Tested only on linux though.
As you can see now osx travis is up and running.
Also included some other tempering with travis and script for build and testing.
Cucumber version uplifted to 1.3.20 as 2.x is not recognizing steps.

hvellyr and others added 23 commits January 28, 2014 00:47
... using a different framework::init() API.
… specific ruby version (using rvm)

Thern run test, features and example.
Travis will now create builds also for osx and 3 different ruby versions.
Travis script part  moved to travis.sh
Travis will now use ubuntu trusty (14.04) for linux build
As it contains various fixes, which might be helpful
@konserw konserw changed the title Use externalproject for GMock and GTest + cucmber 1.3.20 Setup OSX CI & use externalproject for GMock and GTest & cucmber 1.3.20 Jan 11, 2016
@konserw konserw mentioned this pull request Jan 13, 2016
@paoloambrosio
Copy link
Member

Thanks for the pull request.

Some commits in this pull request are very good, others need work (e.g. the build runs, fails but it is marked as successful). The major issue I see is that so many unrelated changes make this pull request very difficult to review. The list of changes in the description should be a smell that this PR should be split.

I'll try and comment more later. In the meantime please take a look at this thread on "Cukes Devs" for more information: https://groups.google.com/forum/#!topic/cukes-devs/xRaGM_AEl4U

Copy link
Member

Choose a reason for hiding this comment

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

what kind of performance improvement do you expect by inlining this? or was it done for a different reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is from pull request #72

Copy link
Member

Choose a reason for hiding this comment

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

don't think comments are needed here: the whitelisting process is quite clear in the Travis documentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course. TBH it was copy-paste from google-test .travis.yml

@paoloambrosio
Copy link
Member

Using GTest/GMock's CMake file was on my list of improvements so I fully support it. It is worth opening an issue (#91) and sending a pull request for that only, but please after we have fixed the build.

Copy link
Member

Choose a reason for hiding this comment

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

it does not seem to work (always runs with 2.2.3), probably because it's a c++ project

Copy link
Member

Choose a reason for hiding this comment

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

After all Ruby deps are there just to run our tests. We shouldn't be concerned with testing multiple Ruby versions. Let's just choose one thst works with rvm (.ruby-version).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - I noticed that it doesn't work. And agree that it is probably because it's not ruby project for travis. I left it there to think about it later, but then forgot about that...
I agree that we shouldn't be concerned with testing multiple Ruby versions.
I believe we can just depend on ruby version preinstalled on CI (if it works)

@paoloambrosio
Copy link
Member

Compiler warning fixes are very good, can you send a separate PR when we are done with the build maintenance?

@konserw
Copy link
Contributor Author

konserw commented Jan 14, 2016

Using GTest/GMock's CMake file was on my list of improvements so I fully support it. It is worth opening an issue (#91) and sending a pull request for that only, but please after we have fixed the build.

OK

Compiler warning fixes are very good, can you send a separate PR when we are done with the build maintenance?

Most of them come from pull request #72 I think.

Some commits in this pull request are very good, others need work (e.g. the build runs, fails but it is marked as successful). The major issue I see is that so many unrelated changes make this pull request very difficult to review. The list of changes in the description should be a smell that this PR should be split.

I'm sorry for this mess - it's my first attempt to upstream any code.
Please just disregard this pull request. I'll try to prepare separate cleaner pull requests.

@konserw konserw deleted the upstream_travis branch September 28, 2016 07:54
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.

4 participants