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

Simplify Travis CI configuration #2575

Merged
merged 4 commits into from
Jun 23, 2017

Conversation

wangkuiyi
Copy link
Collaborator

Fixes #2571

Copy link
Collaborator

@reyoung reyoung left a comment

Choose a reason for hiding this comment

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

There are lines should be removed. I will append some commits to this PR.

# Compile Documentation only.
cmake .. -DCMAKE_BUILD_TYPE=Debug -DCMAKE_Fortran_COMPILER=/usr/bin/gfortran-4.8 -DWITH_GPU=OFF -DWITH_DOC=OFF -DWITH_STYLE_CHECK=OFF ${EXTRA_CMAKE_OPTS}
Copy link
Collaborator

Choose a reason for hiding this comment

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

gfortran is used to compile openblas, which is used by Paddle.

Maybe remove openblas will cause building error.

Copy link
Contributor

Choose a reason for hiding this comment

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

current paddle build openblas does not need gfortran any more.

@@ -18,7 +17,6 @@ addons:
packages:
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this file, JOB=BUILD_AND_TEST should be removed.

# Compile Documentation only.
cmake .. -DCMAKE_BUILD_TYPE=Debug -DCMAKE_Fortran_COMPILER=/usr/bin/gfortran-4.8 -DWITH_GPU=OFF -DWITH_DOC=OFF -DWITH_STYLE_CHECK=OFF ${EXTRA_CMAKE_OPTS}
cmake .. -DCMAKE_BUILD_TYPE=Debug -DWITH_GPU=OFF -DWITH_DOC=OFF -DWITH_STYLE_CHECK=OFF
mkdir output
make -j `nproc`
Copy link
Contributor

@helinwang helinwang Jun 23, 2017

Choose a reason for hiding this comment

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

Why there are two cmake (line 9 and 14) and make (line 11 and 15) when building doc only?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was told once by @QiJune this is because PaddlePaddle relies on SWIG, instead of a C-API, to expose functionalities to Python. But I don't understand the direct reason between SWIG and two runs of CMake.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because part of Paddle's Python code is generated by C++ header. Only after we complete compiling PaddlePaddle, Paddle's Python package can be built. And Sphinx can load this python package to generate documentation.

So, generate documentation needs Python package. Python package needs to compile Paddle.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But why we need to run CMake twice to compile Paddle?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Paddle is using Sphinx to generate documentation. Sphinx will load all Paddle's python package, py_paddle and paddle. Part of py_paddle is not pure Python. It is currently using SWIG to generate and linking all Paddle libraries.

So, we must create py_paddle and paddle Python packages first(the first cmake), and install them to current Python interpreter. And then let sphinx generate all documentations(the second cmake).

If we use C-API to expose Paddle library, then paddle Python packages could be pure Python. And then, we could just use sphinx to generate documentation.

Copy link
Contributor

@helinwang helinwang left a comment

Choose a reason for hiding this comment

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

LGTM, one comment, but not merge blocker.

@reyoung reyoung merged commit bf7a278 into PaddlePaddle:develop Jun 23, 2017
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