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

CMake: Silent git diff after the build, one-click GUI-based config, much cleanup & fixes #75

Merged
merged 10 commits into from Mar 23, 2018

Conversation

tschw
Copy link
Contributor

@tschw tschw commented Mar 22, 2018

See commits for details. Hope you like.

Maybe should also bump the CMake version to 3.3, because that's the version introducing --format=zip as used for packaging pyluxcoretools... I left it alone for now.

@Dade916
Copy link
Member

Dade916 commented Mar 22, 2018

Hi Toby,

Thanks for the patch. I testing the new cmake files on Linux but they don't work. I think it is just missing a "\n" at the end of .cpp files generated from .cl.

Checking.

@Dade916
Copy link
Member

Dade916 commented Mar 22, 2018

Yup, the last line of any .cpp generated from a .cl looks like:

"uint AtomicAddMod(__global uint *val, const uint delta, const uint mod) {\n"
"	uint oldVal, newVal;\n"
"\n"
"	do {\n"
"		oldVal = *val;\n"
"		newVal = (oldVal + delta) % mod;\n"
"	} while (atomic_cmpxchg((__global uint *)val, oldVal, newVal) != oldVal);\n"
"\n"
"	return oldVal;\n"
"}";
} }

The very last string is missing a "\n" and this is a problem when the string are concatenated, you get something like:

"luxrays_types.cl", line 634: error: expected a declaration
  } BVHArrayNode;#line 2 "bvh_kernel.cl"

because the missing "\n" before "#line" of the next concatenated string. Doesn't it happen on Windows too (you have to run an OpenCL rendering to get the error because the code is compiled at runtime) ?

@tschw
Copy link
Contributor Author

tschw commented Mar 22, 2018

Yep, let me fix that.

Doesn't it happen on Windows too (you have to run an OpenCL rendering to get the error because the code is compiled at runtime) ?

Most probably does.

It had gotten super late once I had the PR together, so I didn't do run time tests, especially since there's CI in place (but looking closer, I notice that it does CPU-only testing). I will also check on whether those CMake warnings come from something I've touched.

I guess the actual reason for CI failure

The job exceeded the maximum time limit for jobs, and has been terminated.

is due to load (daytime in the USA).

@Dade916
Copy link
Member

Dade916 commented Mar 22, 2018

I guess the actual reason for CI failure

The job exceeded the maximum time limit for jobs, and has been terminated.

is due to load (daytime in the USA).

Yeah, TravisCI is nearly useless at the moment, I have reduce the number of unit tests executed there but the combo of slow VM and too much intensive unit tests hits the time limit very often.

@tschw
Copy link
Contributor Author

tschw commented Mar 22, 2018

The very last string is missing a "\n"

In-place edit of commit Re-implemented kernel preprocessor with CMake:

diff --git a/cmake/Scripts/PreprocessKernel.cmake b/cmake/Scripts/PreprocessKernel.cmake
index 98518b4d..dc9c1ff1 100644
--- a/cmake/Scripts/PreprocessKernel.cmake
+++ b/cmake/Scripts/PreprocessKernel.cmake
@@ -9,6 +9,6 @@ string(REGEX REPLACE "\"" "\\\\\"" CL_SOURCE_CODE "${CL_SOURCE_CODE}")

 # Wrap lines in string literals:
 string(REGEX REPLACE "\r?\n$" "" CL_SOURCE_CODE "${CL_SOURCE_CODE}")
-string(REGEX REPLACE "\r?\n" "\\\\n\"\n\"" CL_SOURCE_CODE "\"${CL_SOURCE_CODE}\"")
+string(REGEX REPLACE "\r?\n" "\\\\n\"\n\"" CL_SOURCE_CODE "\"${CL_SOURCE_CODE}\\n\"")

 configure_file(${template} ${DST} @ONLY)

As for the warnings, one is from FindOpenMP.cmake shipped with CMake versions below 3.10.
It only happens when targeting CMake version < 3.1.

https://gitlab.kitware.com/cmake/cmake/issues/17292

The other is from my code, because pyside-uic is not found. Is it installed on the build host? If so, where to best look for it on Linux?

Yeah, TravisCI is nearly useless at the moment, I have reduce the number of unit tests executed there but the combo of slow VM and too much intensive unit tests hits the time limit very often.

Maybe could gain a bit of headroom for testing by saving build time by using ninja... It builds roughly twice as fast as MSBuild, which is already quite a bit faster than GNU-Make, I think.

@Theverat
Copy link
Member

Theverat commented Mar 22, 2018

The other is from my code, because pyside-uic is not found. Is it installed on the build host? If so, where to best look for it on Linux?

On Ubuntu, I used the following to install it:
sudo pip3 install --install-option="--jobs=8" PySide
(jobs=n can of course be changed to your processor count)
I recommend to install it multithreaded, otherwise it takes ages to compile.

endif()

find_program(PYSIDE_UIC NAME pyside-uic
HINTS "${PYTHON_INCLUDE_DIRS}/../Scripts"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will that hint work for all platforms?

Copy link
Member

Choose a reason for hiding this comment

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

Yup, pyside-uic is in system path on linux by default.

@Dade916 Dade916 merged commit 9e3d04a into LuxCoreRender:master Mar 23, 2018
@Dade916
Copy link
Member

Dade916 commented Mar 23, 2018

Thanks Toby, it works fine here, I have merged the pull request.

@tschw
Copy link
Contributor Author

tschw commented Mar 23, 2018

This was really just scratching the tip of the iceberg. There is much more that could be done on the CMake end, such as declaring installation rules to generate the SDK in a cross-platform fashion, or an automated, external build for the dependencies (that would allow me to easily pull up to VisualStudio 2017 and get rid of yet another version of that massive software package requiring gigabytes of disk space).

Using target exports it becomes unnecessary to directly fiddle with find scripts, paths and libraries: Instead you just have your projects depend on targets generated by external builds as if they were local ones.

I've become bit of a "CMake power user" over the recent years (it's how I stay halfway sane targeting both UNIX and Windows) and can help with that once it fits into our schedule.

@Dade916
Copy link
Member

Dade916 commented Mar 23, 2018

Yes but keep in mind that, given the limited resources, we have a bit to prioritize our work. Any change to the building process requires to test about 10+ different builds: CPU-only/With OpenCL support, Linux/Windows, Stand-alone/BlendLuxCore/SDK/Python wheel builds, static linking/DLLs ... it is a lot of work so we usually esitate to change building process once we have something working.

Something I would like to have (and use) is separate directories for DEBUG and RELEASE builds. At the moment, when I switch from RELEASE to DEBUG, I have to recompile everything while, with different directories, I would have a way to reuse, at least some object from a previous compilation.

However a change like this would require separate DEBUG/RELEASE bin and lib directories and it would impact all the scripts for packing the aforementioned releases. Even a small change in this area lead to a lot of (annoying) testing.

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.

None yet

3 participants