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

Jump to bottom

THRIFT-5109: Misc CMake build improvements #2094

Closed
wants to merge 2 commits into from
Closed

THRIFT-5109: Misc CMake build improvements #2094

wants to merge 2 commits into from

Conversation

emmenlau
Copy link
Member

@emmenlau emmenlau commented Apr 8, 2020

Patch: Mario Emmenlauer

This closes #2026
This closes #2025
This closes #2021

  • https://issues.apache.org/jira/browse/THRIFT-5109
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
  • Did you squash your changes to a single commit? (not required, but preferred)
  • Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
  • If your change does not involve any code, add [skip ci] at the end of your pull request to free up build resources.

@emmenlau emmenlau force-pushed the bda_misc_cmake_build_improvements branch 7 times, most recently from e962146 to 814a6ba Compare April 15, 2020 09:35
@jeking3
Copy link
Contributor

jeking3 commented Apr 17, 2020

Moving towards one build system (cmake) is a great effort, it's nice to see this happening in some capacity. Unfortunately when builds break... merging is hard. :)

@emmenlau
Copy link
Member Author

Unfortunately when builds break... merging is hard. :)

Haha yes! I was actually hoping you could help with the Gruntfile?

The "problem" now is not that the CMake build itself would be broken. The problem is that the Javascript build in autotools was not used on MSVC before, whereas in CMake I have now enabled Javascript on MSVC. And it fails, probably never worked.

This could and should be split off into a separate issue, unless you (or someone else) could fix the Javascript build?

@emmenlau
Copy link
Member Author

The rest of this PR should be fine and should have worked from the start. The failing Javascript could just be disabled on AppVeyor.

@janosvitok
Copy link
Contributor

I was going to suggest the same thing, but was distracted and didn't get back to it.

Since JS issue is unrelated to cmake (the issue is that JS can't be compiled on windows), one solution is to disable JS build on windows, and make another issue for JS.

@emmenlau emmenlau force-pushed the bda_misc_cmake_build_improvements branch 3 times, most recently from cfc83c0 to 697fe3e Compare April 20, 2020 09:30
@emmenlau
Copy link
Member Author

I've found a small improvement where the Javascript target actually depends on the target that copies the compiler into the source tree, not on the build of the compiler. This ensures that if the compiler is not built, it will still be copied.

If the CI is now successful, this should be ready for review / merge.

@emmenlau
Copy link
Member Author

There seems to be an unrelated issue in https://travis-ci.org/github/apache/thrift/jobs/677163097. Should I re-trigger the builds and tests?

@emmenlau emmenlau force-pushed the bda_misc_cmake_build_improvements branch from 697fe3e to 3ca3274 Compare April 20, 2020 12:28
@emmenlau
Copy link
Member Author

Re-triggered build with a rebase on latest master, to fix random error in Travis CI.

@emmenlau
Copy link
Member Author

:-( Another random error in the CI. Can this be merged anyways?

emmenlau and others added 2 commits April 21, 2020 10:37
@emmenlau emmenlau force-pushed the bda_misc_cmake_build_improvements branch from 3ca3274 to 8af9f7b Compare April 21, 2020 08:38
@emmenlau
Copy link
Member Author

Ready for review.

@Jens-G
Copy link
Member

Jens-G commented Apr 21, 2020

@janosvitok comments from your side?

janosvitok
janosvitok approved these changes Apr 22, 2020
View reviewed changes
Copy link
Contributor

@janosvitok janosvitok left a comment

Choose a reason for hiding this comment

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

I can't tell anything about th grunt part; cmake looks OK.
I like the NOT WIN32 solution to disable JS on WIndows.

@Jens-G Jens-G closed this in 93171d2 Apr 22, 2020
@emmenlau emmenlau deleted the bda_misc_cmake_build_improvements branch April 22, 2020 19:59
sveneld pushed a commit to sveneld/thrift that referenced this pull request May 29, 2020
@emmenlau
THRIFT-5109 Misc Cmake build improvements
58850cb 
Patch: Mario Emmenlauer

This closes apache#2094
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janosvitok janosvitok janosvitok approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@emmenlau @jeking3 @janosvitok @Jens-G