-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Conversation
There was a problem hiding this 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} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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` |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
Fixes #2571