Added Windows support via CMake. #40

pkeir · 2018-11-12T18:35:40Z

Tested on Visual Studio 2017.

mmha · 2018-11-26T18:38:46Z

I'd very much like this to be merged. @Oblomov I have seen your comment in #20 and I would like to give a counter-argument:

While adding an additional dependency to build this program may seem to add complexity, the opposite effect is true. Let's compare the current state of building clinfo with using CMake. Currently there are:

2 Makefiles, one for Unix, one for Windows
2 Batch scripts for Windows
1 shell script that works as a code generator

By adopting CMake, you would replace those 5 files with exactly one (+ template for the version header) that works on every operating system CMake supports (and that's quite a lot). CMake has become so ubiquitous that even fairly niche Unix systems like Solaris have it available in their system package manager. In other words, requiring users to install it is almost the same as just installing make itself at this point.

This alone removes a lot of code maintenance burden.

The other important point is package management, both consuming OpenCL and packaging clinfo itself.

Consuming OpenCL

For Windows there is fetch-opencl-dev-win.cmd to fetch the dependency. This homebrew approach is very fragile because it makes a lot of assumptions that make development on Windows harder:

I used to work at a company that enforced a HTTP(S) proxy. You couldn't "just" download something. I have fond memories of fiddling with curl's environment variables and flags to ignore the MITM TLS certificate and patiently wait until the web anti-virus scanner completes. Fortunately I have switched workplaces since, but those companies do exist and developers from those companies will come here and ask for help with their broken IT if you provide such a script.
vcpkg is the Microsoft-sanctioned way of getting dependencies. It contains a script for the OpenCL ICD + headers already. Which brings us to the next problem:
There is no ergonomic way of using OpenCL if the install prefix differs. In Linux Makefile land there is PkgConfig, but it wouldn't solve the problem for Windows.
- This point is especially important to me: I need to cross-compile software regularly. I have created my own OpenCL package for Conan so I can easily switch between compiler toolchains. With CMake, this Just Works (tm) but with custom Makefiles I would also need to write custom code to inject the path to the cross-compiled OpenCL. I would also need to do the same if I want to compile with a version of OpenCL different from the system one.
You as the maintainer are responsible for correctly versioning your dependencies. Again, this is unnecessary maintenance overhead. As it stands right now, you're downloading tip-of-trunk headers and an OpenCL 1.2 ICD loader on Windows. Such version mismatches can easily be avoided by using vcpkg, just like this PR does.

Packaging clinfo

While system-specific package managers like PKGBUILD can build clinfo relatively easy, this is not the case anymore when using cross-platform language package managers like vcpkg or conan. You'd need to special case every operating system because there are subtle incompatibilities between the Makefiles hidden everywhere. For example, the Windows Makefile lacks an install rule. And I'd argue that issues like this will occur again and again as long as multiple build systems for different operating systems are maintained. Writing build scripts is hard, writing Makefiles that cover every corner case even more so. Abstraction is key.

Last but not least: Adding a CMakeLists doesn't remove anything. It merely adds a new build system with all disadvantages of additional code, but the old stuff keeps working until removed.

This is the second PR for adding a CMake build system and the third person asking for it. If people wouldn't have issues with the current situation they wouldn't keep submitting patches. I hope you reconsider your position.

MathiasMagnus · 2019-01-24T13:06:11Z

First of all I would like to congratulate @Oblomov on the utility. I arrived at this repo checking the exact clinfo Canonical packages for Ubuntu. Second of all, very big yes for CMake! I understand the minimalistic approach, but do agree with @mmha that having another way of building doesn't have to interfere with the other methods. CMake has become the de facto standard of building cross-platform software (unfortunately). We might as well make the most of the situation and use what CMake has to offer.

I am the maintainer of the OpenCL dev package for Vcpkg and I would like to enhance the package with minimal utilities, such as clinfo. Whether the scripts are mainlined or not doesn't really concern me, it all boils down to having to submit the build script as a patch or not.

As far as the script goes, I would like to propose a little bikeshedding…

MathiasMagnus · 2019-01-24T13:08:34Z

CMakeLists.txt

@@ -0,0 +1,19 @@
+cmake_minimum_required(VERSION 3.6)
+
+if (WIN32 AND DEFINED ENV{VCPKG_ROOT})


This should not be here. The entire point of Vcpkg is that the build scripts can be agnostic of using Vcpkg or not. Vcpkg is not Windows only, so there is no reason to check for targeting Windows or not.

Fair enough. I've removed the WIN32 component there.

MathiasMagnus · 2019-01-24T13:11:40Z

CMakeLists.txt

+  set(CMAKE_TOOLCHAIN_FILE "$ENV{VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmake")
+endif ()
+
+set(n clinfo)


I would not be so lazy and rather use ${CMAKE_PROJECT_NAME} when referring later to the name. At least make it something memorable, like ${Name}. (I tend to use different capitalization for temporary vars.)

You mean n? I like it as it is, and it's nothing to do with laziness. You can suggest the change you suggest if the owner accepts.

Single letter variable names are not great. I'd just use clinfo everywhere instead of ${n}.

I agree with @ProGTX. Just writing clinfo everywhere is much clearer than a cryptic ${n}

MathiasMagnus · 2019-01-24T13:17:54Z

CMakeLists.txt

+target_include_directories(${n} PUBLIC ${OpenCL_INCLUDE_DIRS})
+target_link_libraries(${n} ${OpenCL_LIBRARIES})
+
+if (WIN32)


The property is not Windows specific but generator specific. If you want to guard it, correct check is

if (CMAKE_GENERATOR MATCHES "Visual Studio")

However, I would omit the check, as this directory property has no effect on generators other than VS. Perhaps a comment might suffice. If you really would like to cater to IDE users, I'd also add source_group properties for Filters in the project.

I've omitted the check. I'll leave you to suggest a source_group properties addition. I think they look ugly on small projects, and we don't need complexity at this stage in negotiations with the repo owner.

…ored.

ProGTX · 2019-01-25T10:01:51Z

Thank you, I'd very much like to see this merged as well. Building with Makefiles is a pain, and this is much more portable - VS2017 supports CMake out of the box. I just use my own fork of the repo when I need to build it.

ProGTX

Thank you for this. The only thing that has to be added is CMAKE_DL_LIBS - I don't think it works on Linux without that.

ProGTX · 2019-01-25T09:59:08Z

CMakeLists.txt

+  set(CMAKE_TOOLCHAIN_FILE "$ENV{VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmake")
+endif ()
+
+set(n clinfo)


Single letter variable names are not great. I'd just use clinfo everywhere instead of ${n}.

ProGTX · 2019-01-25T10:03:13Z

CMakeLists.txt

+add_executable(${n} src/${n}.c)
+target_include_directories(${n} PUBLIC src)
+target_include_directories(${n} PUBLIC ${OpenCL_INCLUDE_DIRS})
+target_link_libraries(${n} ${OpenCL_LIBRARIES})


I think you need to link against CMAKE_DL_LIBS as well, if available. That's what the Makefile does.

This is not needed.

It is on Linux:

[2/2] Linking C executable clinfo FAILED: : && /usr/lib/ccache/cc CMakeFiles/clinfo.dir/src/clinfo.c.o -o clinfo /usr/lib/libOpenCL.so && : /usr/bin/ld: error: undefined symbol: dlsym >>> referenced by clinfo.c >>> CMakeFiles/clinfo.dir/src/clinfo.c.o:(oclIcdProps) collect2: error: ld returned 1 exit status ninja: build stopped: subcommand failed.

You are of course correct :) I've added the change you suggest.

ProGTX · 2019-01-25T10:04:25Z

CMakeLists.txt

+set(n clinfo)
+project(${n})
+
+find_package(OpenCL REQUIRED)


Another sensible requirement would be CMAKE_C_STANDARD, 99 works.

I suggest we keep this file simple for now. I hope for the repo owner to recognise how minor this addition is.

That's fine by me

mmha · 2019-01-30T00:10:34Z

CMakeLists.txt

+add_executable(${n} src/${n}.c)
+target_include_directories(${n} PUBLIC src)
+target_include_directories(${n} PUBLIC ${OpenCL_INCLUDE_DIRS})
+target_link_libraries(${n} ${OpenCL_LIBRARIES} ${CMAKE_DL_LIBS})


If we require CMake 3.7 we could use an imported target instead and get rid of that include directory:

target_link_libraries(${n} PRIVATE OpenCL::OpenCL)

In any case, always use the PRIVATE/PUBLIC/INTERFACE keywords with target_link_libraries. It is incompatible with the new syntax.

mmha · 2019-01-30T00:13:04Z

CMakeLists.txt

+find_package(OpenCL REQUIRED)
+
+add_executable(${n} src/${n}.c)
+target_include_directories(${n} PUBLIC src)


This can be simplified to a single function call:

target_include_directories(${n} PRIVATE src ${OpenCL_INCLUDE_DIRS})

If we use an imported target (see below), this comment can obviously be ignored.

mmha · 2019-01-30T00:17:12Z

CMakeLists.txt

+cmake_minimum_required(VERSION 3.6)
+
+if (DEFINED ENV{VCPKG_ROOT})
+  set(CMAKE_TOOLCHAIN_FILE "$ENV{VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmake")


Toolchain files are evaluated very early, even before all the compiler checks. Setting this variable has no effect at all, it is supposed to be passed on the command line.

Did you mean to include() this file instead?

This is not my PR, but honestly I'm still very much against adding automagic Vcpkg detection for multiple reasons:

It cannot be turned off. If I wanted to pick up an OpenCL version other than what ships with Vcpkg it is impossible if it's hardcoded in the script.

It is unorthodox. It goes against the biggest feature of Vcpkg, that scripts need not know of it's existence. That is the primary reason why Vcpkg is better than Conan. Scripts need to be Conan aware, while using Vcpkg is transparent to downstreams.

Users of Vcpkg already have a way of not spamming the command-line with toolchain file specification. If they use an IDE, in VS the toolchain file can be set in CMakeSettings.json (now with GUI) in a set-it-and-forget it manner once and for all. VS Code users are probably using CMake Tools and set the toolchain file via Kits, again: set-it-and-forget-it. Even if someone is a console junkie on Linux (and not using VS Code on Linux), they probably got tired of -D CMAKE_TOOLCHAIN_FILE=$(VCPKG_ROOT)/scripts/buildsystems/vcpkg.cmake ... so they most likely have an alias set that adds this one variable to the invocation.

Long story short, I don't see the benefit of this, other than alienating non-Vcpkg users of ever reyling on it by giving the impression the scripts need to use this or just simply plaguing projects with it. Removing makes the script shorter, leaner and nicer in every regard.

I don't mind either way, but you can always override the default search paths for a package by passing -DOpenCL_ROOT=C:/myopencl. But speaking as a Conan user, I don't like the idea of using CMAKE_TOOLCHAIN_FILE to inject package management like this. How does cross-compilation work with vcpkg?

That is the primary reason why Vcpkg is better than Conan. Scripts need to be Conan aware, while using Vcpkg is transparent to downstreams.

This is OT, but Conan's cmake_paths generator integrates exactly like vcpkg.

How does cross-compilation work with vcpkg?

Either our toolchain can be pulled in at the bottom of your own toolchain, or you can ask us to chainload yours via -DVCPKG_CHAINLOAD_TOOLCHAIN_FILE=xyz.

mmha · 2019-01-30T00:18:57Z

CMakeLists.txt

+  set(CMAKE_TOOLCHAIN_FILE "$ENV{VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmake")
+endif ()
+
+set(n clinfo)


I agree with @ProGTX. Just writing clinfo everywhere is much clearer than a cryptic ${n}

pkeir added 4 commits November 12, 2018 18:29

Added Windows support via CMake.

710523c

Avoid a CMake error of VCPKG_ROOT is undefined.

4e878e1

Avoid a CMake error if VCPKG_ROOT is undefined.

9f34f38

Merge branch 'master' of https://github.com/pkeir/clinfo

6bcb49a

MathiasMagnus reviewed Jan 24, 2019

View reviewed changes

pkeir added 2 commits January 24, 2019 23:39

No need to check for WIN32 *and* VCPKG_ROOT

4a7fc82

No need for WIN32 or VS. In other contexts the property is simply ign…

9c68f1e

…ored.

ProGTX suggested changes Jan 25, 2019

View reviewed changes

dlsym on Linux needs the -ldl.

660dbc1

mmha reviewed Jan 30, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Windows support via CMake. #40

Added Windows support via CMake. #40

pkeir commented Nov 12, 2018

mmha commented Nov 26, 2018

MathiasMagnus commented Jan 24, 2019

MathiasMagnus Jan 24, 2019

pkeir Jan 24, 2019

MathiasMagnus Jan 24, 2019

pkeir Jan 24, 2019

ProGTX Jan 25, 2019

mmha Jan 30, 2019

MathiasMagnus Jan 24, 2019

pkeir Jan 24, 2019

ProGTX commented Jan 25, 2019

ProGTX left a comment

ProGTX Jan 25, 2019

ProGTX Jan 25, 2019

pkeir Jan 25, 2019

ProGTX Jan 25, 2019

pkeir Jan 28, 2019

ProGTX Jan 25, 2019

pkeir Jan 25, 2019

ProGTX Jan 25, 2019

mmha Jan 30, 2019

mmha Jan 30, 2019

mmha Jan 30, 2019

MathiasMagnus Jan 30, 2019

mmha Jan 30, 2019 •

edited

Loading

ras0219-msft Jan 30, 2019

mmha Jan 30, 2019

		@@ -0,0 +1,19 @@
		cmake_minimum_required(VERSION 3.6)

		if (WIN32 AND DEFINED ENV{VCPKG_ROOT})

Added Windows support via CMake. #40

Are you sure you want to change the base?

Added Windows support via CMake. #40

Conversation

pkeir commented Nov 12, 2018

mmha commented Nov 26, 2018

Consuming OpenCL

Packaging clinfo

MathiasMagnus commented Jan 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ProGTX commented Jan 25, 2019

ProGTX left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmha Jan 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmha Jan 30, 2019 •

edited

Loading