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

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

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

Conversation

nikhiljnv
Copy link
Contributor

Add CONTRIBUTING.md to explain

  1. How to contribute to OpenCL ICD Loader project.
  2. ICD loader versioning guidelines including
    when to increment the version number.

Add CONTRIBUTING.md to explain
1. How to contribute to OpenCL ICD Loader project.
2. ICD loader versioning guidelines including
   when to increment the version number.
@CLAassistant
Copy link

CLAassistant commented Jun 18, 2019

CLA assistant check
All committers have signed the CLA.

CONTRIBUTING.md Outdated
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

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

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

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

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.

@bashbaug bashbaug added the agenda Needs Working Group Discussion label Jul 1, 2019
Add instructions to increment RELEASE field of
linux ICD loader library version.
@nikhiljnv
Copy link
Contributor Author

Updated the document with linux versioning suggested above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agenda Needs Working Group Discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants