Skip to content

WIP: Add CONTRIBUTING.md and explain version increment #75

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.

Sign up for GitHub

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

Open
nikhiljnv wants to merge 2 commits into
base: main
Choose a base branch
from
Open

WIP: Add CONTRIBUTING.md and explain version increment #75

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
926eeaa
WIP: Add CONTRIBUTING.md and explain version increment
nikhiljnv Jun 18, 2019
70fe7c6
Update CONTRIBUTING.md for Linux ICD loader version
nikhiljnv Jul 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# Contributing to the OpenCL ICD Loader project.

Contributions to the OpenCL ICD Loader are welcomed and encouraged.
Please follow the instructions below for submitting your patches.

## Preparing and submitting changes

Please propose changes to the ICD loader using GitHub pull requests.
You will be prompted with a one-time "click-through" CLA dialog as part of submitting your pull request or other contribution to GitHub.

## Windows DLL versioning

The ICD Loader version is maintained in `loader/windows/OpenCL.rc` in the form `x.y.z.0` in three fields:
FILEVERSION, PRODUCTVERSION and FileVersion.

The `z` component of the version must be incremented by one for each pull request that modifies anything in the `loader` directory.
The fields must match each other after the increment.

## Linux shared library versioning

The ICD loader library version on Linux is 1.2 with SONAME set to libOpenCL.so.1 using the SOVERSION property in CMakeLists.txt.
This version does not change when new API functions are added to the ICD loader because the loader maintains backwards compatibility.
Note that the version number is independent of the OpenCL specification revision number.

No changes are required in the library version when submitting pull requests.
Copy link
Contributor

@bashbaug bashbaug Jun 20, 2019

Choose a reason for hiding this comment

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

Is there a reason why we wouldn't want to update the Linux library version when making pull requests?

If we don't update the Linux library version then a) how can we ensure a newer ICD loader with a critical bugfix will overwrite an older ICD loader without the bugfix, or conversely b) how can we ensure that an older ICD loader will not overwrite a newer ICD loader?

Copy link
Contributor

@kepatil kepatil Jul 19, 2019

Choose a reason for hiding this comment

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

We probably should increment version for Linux too. However, Linux does not seem to have an exact equivalent of Windows FileVersion embedded inside the library file.

Instead I see a filename versioning convention based on symlinks, like libfoo.MAJOR -> libfoo.MAJOR.MINOR -> libfoo.MAJOR.MINOR.RELEASE. We could perhaps increment RELEASE with each change.

However, I don't know of an authoritative reference for this scheme. Also, I don't know how to achieve this using CMake. Any pointers would be helpful.

Copy link
Contributor

@bashbaug bashbaug Jul 22, 2019

Choose a reason for hiding this comment

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

This sounds reasonable.

We have this in our CMake file today:

set_target_properties (OpenCL PROPERTIES VERSION "1.2" SOVERSION "1")

The VERSION line controls which symlinks are created. With our current CMake file we get:

$ ls -l libOpenCL.so*
lrwxrwxrwx 1 bashbaug bashbaug    14 Jul 22 16:48 libOpenCL.so -> libOpenCL.so.1
lrwxrwxrwx 1 bashbaug bashbaug    16 Jul 22 16:48 libOpenCL.so.1 -> libOpenCL.so.1.2
-rwxrwxr-x 1 bashbaug bashbaug 44784 Jul 22 16:48 libOpenCL.so.1.2

We can add a "release" version and it will give us:

$ ls -l libOpenCL.so*
lrwxrwxrwx 1 bashbaug bashbaug    14 Jul 22 16:46 libOpenCL.so -> libOpenCL.so.1
lrwxrwxrwx 1 bashbaug bashbaug    18 Jul 22 16:46 libOpenCL.so.1 -> libOpenCL.so.1.2.0
-rwxrwxr-x 1 bashbaug bashbaug 44784 Jul 22 16:46 libOpenCL.so.1.2.0

Note that the "soversion" is different and gets embedded into the file itself:

$ objdump -p libOpenCL.so.1.2 | grep SONAME
  SONAME               libOpenCL.so.1

If we set the CMake project version we could probably use that, both for Windows and Linux, so we'd only need to change the version in one place:

https://cmake.org/cmake/help/latest/command/project.html

I suspect there's a way to generate a new patch version for each git commit but I haven't found a way to do that just yet.

Copy link
Contributor

@bashbaug bashbaug Jul 24, 2019

Choose a reason for hiding this comment

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

I think the updated text is a step in the right direction, but I think the documentation should describe what our convention is, and not what it should be:

ICD loader library version should take the form MAJOR.MINOR.RELEASE...

If we think our convention should be different than what we are doing right now (say, that it should include a RELEASE version) then let's fix it.