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.

Sign up for GitHub

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

Jump to bottom

THRIFT-4732:refine cmake scripts #1688

Merged
merged 3 commits into from
Feb 7, 2019
Merged

THRIFT-4732:refine cmake scripts #1688

merged 3 commits into from
Feb 7, 2019

Conversation

cyyever
Copy link
Contributor

@cyyever cyyever commented Jan 12, 2019 •
edited
Loading

1.add -wall option for all major compilers.
2.add cmake support in test/c_glib
3.add static analysis targets

jeking3
jeking3 requested changes Jan 14, 2019
View reviewed changes
Copy link
Contributor

@jeking3 jeking3 left a comment

Choose a reason for hiding this comment

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

Based on the unconditional dynamic linking of boost, I'm marking this as request changes.

@jeking3
Copy link
Contributor

jeking3 commented Jan 22, 2019

On the appveyor build, somehow it's trying to pick up the statically linked boost locale when all the other ones are dynamic. Did you change anything related to that?

On the Travis CI side, you need to rebase on master to pick up the appropriate dart changes to fix the cross test build.

jeking3
jeking3 requested changes Jan 22, 2019
View reviewed changes
Copy link
Contributor

@jeking3 jeking3 left a comment

Choose a reason for hiding this comment

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

Some issues with linking need to be addressed on Windows.

@jeking3 jeking3 added the releng Release Engineering label Jan 22, 2019
@jeking3 jeking3 self-assigned this Jan 22, 2019
@cyyever
Copy link
Contributor Author

cyyever commented Jan 23, 2019

Some issues with linking need to be addressed on Windows.

Have you ever tried to build shared library of master branch successfully on windows? I have encountered some unresolved external symbol errors even with CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS enabled.After spending several hours to investigate this problem,I found that we still need to put dllimport and dllexport in some places.

@jeking3
Copy link
Contributor

jeking3 commented Jan 25, 2019

See https://issues.apache.org/jira/browse/THRIFT-4759

@cyyever
Copy link
Contributor Author

cyyever commented Jan 26, 2019

See https://issues.apache.org/jira/browse/THRIFT-4759

I would rebase this PR after THRIFT-4759 is solved.

@jeking3
Copy link
Contributor

jeking3 commented Feb 6, 2019

THRIFT-4759 was solved, please rebase and continue. :)

jeking3
jeking3 reviewed Feb 7, 2019
View reviewed changes
@jeking3 jeking3 merged commit a6a3a78 into apache:master Feb 7, 2019
@jeking3
Copy link
Contributor

jeking3 commented Feb 7, 2019

Does this close THRIFT-4732 or is there more work to do?

@cyyever
Copy link
Contributor Author

cyyever commented Feb 7, 2019

Does this close THRIFT-4732 or is there more work to do?

To complete THRIFT-4732,I need another PR.

@cyyever cyyever deleted the refine_cmake branch February 20, 2019 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeking3 jeking3 jeking3 approved these changes

Assignees

@jeking3 jeking3

Labels
releng Release Engineering
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cyyever @jeking3