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

Get rid of the dependency of Go compiler when WITH_GOLANG is OFF. #8610

Merged
merged 5 commits into from
Feb 28, 2018

Conversation

Xreki
Copy link
Contributor

@Xreki Xreki commented Feb 27, 2018

In CMake, project is used to set a name, version, and enable languages for the entire project.

We can call enable_language to enable the Go language optionally.

It can fix problems in #8555 #5427 #3741 etc.

@Xreki Xreki requested a review from luotao1 February 27, 2018 12:10
@@ -58,7 +58,7 @@ Users can specify the following Docker build arguments with either "ON" or "OFF"
| `WITH_AVX` | OFF | Set to "ON" to enable AVX support. |
| `WITH_TESTING` | OFF | Build unit tests binaries. |
| `WITH_MKL` | ON | Build with [Intel® MKL](https://software.intel.com/en-us/mkl) and [Intel® MKL-DNN](https://github.com/01org/mkl-dnn) support. |
| `WITH_GOLANG` | ON | Build fault-tolerant parameter server written in go. |
| `WITH_GOLANG` | OFF | Build fault-tolerant parameter server written in go. |
Copy link
Contributor

Choose a reason for hiding this comment

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

May also change paddle/scripts/docker/build.sh to make this default OFF

Copy link
Contributor Author

@Xreki Xreki Feb 28, 2018

Choose a reason for hiding this comment

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

-DWITH_GOLANG=${WITH_GOLANG:-ON} \

If changing the above value to OFF, then CI cannot test the go codes? Maybe I should change the value in README.md back to ON?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can configure CI to enable golang explicitly to run the tests, the defaults can be still OFF.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

@typhoonzero typhoonzero left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@Xreki Xreki merged commit f054867 into PaddlePaddle:develop Feb 28, 2018
@Xreki Xreki deleted the build_remove_go_if_noneed branch February 28, 2018 03:45
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