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

Update gtest version to 1.8.1 #478

Merged
merged 3 commits into from
Jan 17, 2019
Merged

Update gtest version to 1.8.1 #478

merged 3 commits into from
Jan 17, 2019

Conversation

jslee02
Copy link
Member

@jslee02 jslee02 commented Jan 14, 2019

This also fixes the build failure on Ubuntu Bionic.


Before merging a pull request

  • Set version target by selecting a milestone on the right side
  • Summarize this change in CHANGELOG.md

@jslee02 jslee02 added this to the Aikido 0.3.0 milestone Jan 14, 2019
@brianhou
Copy link
Contributor

Were there any changes to code that we've written, or is the only change to update gtest code? If it's the latter, what's the rationale for not just using a git submodule?

@jslee02
Copy link
Member Author

jslee02 commented Jan 14, 2019

I believe there is one thing we changed to suppress build warnings by adding SYSTEM option to include_directories(...) (see this diff). Let me make the change for this PR as well.

Alternatively, git submodule can be used with custom build script instead of using the provided one by gtest, if git submodule is preferred.

@brianhou
Copy link
Contributor

Hmm, this actually makes me mildly concerned. What if we update gtest again in the future, and don't remember that that change (or other future changes we make) were necessary?

New proposal: what if we fork gtest into PRL, and apply our changes. We can then merge in new versions when desired, while maintaining our own changes. I'm somewhat fuzzy on exactly where libherb and other libraries that use aikido find gtest, but maybe that could be useful for those as well?

@jslee02
Copy link
Member Author

jslee02 commented Jan 14, 2019

Hmm, this actually makes me mildly concerned. What if we update gtest again in the future, and don't remember that that change (or other future changes we make) were necessary?

Agreed.

The proposal sounds reasonable to me.

As a note, libherb download gtest (using git) when it gets built instead of having the code inside the repo. I personally don't prefer this way because PPA doesn't allow to download code while building the package.

Anyways, I don't have any strong preference for this. Any method that is convenient to maintain would be great.

@jslee02 jslee02 mentioned this pull request Jan 14, 2019
5 tasks
Copy link
Contributor

@brianhou brianhou left a comment

Choose a reason for hiding this comment

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

Let's merge for now so we can unbreak Travis. @jslee02 can you make an issue to figure out how to deal with this? Thanks!

@jslee02 jslee02 merged commit 88a7324 into master Jan 17, 2019
@jslee02 jslee02 deleted the gtest_1.8.1 branch January 17, 2019 00:09
@jslee02 jslee02 mentioned this pull request Jan 17, 2019
2 tasks
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