-
Notifications
You must be signed in to change notification settings - Fork 5.9k
Simplify Travis CI configuration #2575
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
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.
| - JOB=PRE_COMMIT | ||
| addons: | ||
| apt: | ||
| 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.
| 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.
helinwang
left a comment
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