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

Add library versioning and mark version 1.0.0-rc3 #3311

Merged
merged 1 commit into from
Jan 23, 2016

Conversation

lukeyeager
Copy link
Contributor

Close #486, Close #2969, Close #3015, Close #3409

Originally NVIDIA#39 and NVIDIA#46

Included

Adds versioning to:

  • shared library (libcaffe.so)
    • both build systems
  • caffe binary (caffe --version)
  • python module (caffe.__version__)

TODO

  • matlab versioning (help?)

For discussion

  • I'm assuming you want to be able to break backwards compatibility with MINOR releases (which means not following semantic versioning to the letter). If you want to go all the way with semver, we should set the SONAME to libcaffe.MAJOR instead of libcaffe.MAJOR.MINOR.
  • Instead of this approach, you could follow the CMake stubs for how to do it with a header file - https://github.com/BVLC/caffe/blob/5a302a2b3b/cmake/Summary.cmake#L79-L84. I went this route due to some extra requirements for NVcaffe.

@lukeyeager
Copy link
Contributor Author

/cc @cdluminate for concerns w.r.t. debian packaging

/cc @Nerei for thoughts about the header file approach from #1667

@cdluminate
Copy link
Contributor

Well, that seems fine to me, and I'd be glad to import the officially versioned Caffe into the Debian package, with a terse version string like -1.0.0 rather than a snapshot version string like -0.9999~rc2+git2015MMDD-gABCDEFA. After importing it, likely that I just need to bump some numbers.

Thank you for working on versioning it :-)

By the way, Caffe itself currently doesn't block packaging at all (it was completed). The real trouble and the real blocker is CUDA from Debian Unstable/Experimental, which is quite out of date (6.5.14 currently) so that it refuses to work with GCC-5. And that is what I'm looking into recently.

@shelhamer
Copy link
Member

@ronghanghu given your matcaffe contributions do you know how to mark the version in MATLAB?

@ronghanghu
Copy link
Member

Hi, @shelhamer, I think for matlab we could simply do caffe.version similar to python. This should be quite easy.

@shelhamer
Copy link
Member

Thanks @lukeyeager! This + the simple matcaffe version extension by @ronghanghu looks like all the infrastructure needed to declare a version.

@ronghanghu
Copy link
Member

I think we can merge this PR first, and I will create another PR for matcaffe.

@lukeyeager
Copy link
Contributor Author

Rebased after [my] conflicting change from #3127.

Any comments on the "for discussion" points above? I agree the matlab versioning can come later.

@flx42
Copy link
Contributor

flx42 commented Nov 11, 2015

I think it's great, but for the future I would like to raise awareness on the possibility of creating some kind of config file for caffe (for instance caffe/config.h). This file would store all the relevant configuration variables that were used to build caffe.
When you use the C++ API of Caffe and include caffe.hpp, there is no way of knowing how the code was compiled. So you might end up using the same header but with a different environment that the one used when the library was compiled. This can break pretty badly if your code and the library disagree on the layout of the objects. For instance we have an example of this bug here:
NVIDIA#30

This file could look like this:

#define CAFFE_VERSION_MAJOR 1
#define CAFFE_VERSION_MINOR 0
#define CAFFE_USE_OPENCV 0
#define CAFFE_USE_CUDNN 1

@jczaja
Copy link
Contributor

jczaja commented Nov 13, 2015

Hi,

I have a question regarding proposed changes. In CMakeLists.txt you are setting CAFFE_TARGET_VERSION and CAFFE_TARGET_SOVERSION. And then add_definitions is passing CAFFE_TARGET_VERSION.So it is like there is a versioning for caffe binary and separate one for caffe lib. This is fine, but in Makefile I can see only DYNAMIC_VERSION_{MAJOR|MINOR|REVISION} and then we have:
COMMON_FLAGS += -DCAFFE_VERSION=$(DYNAMIC_VERSION_MAJOR).$(DYNAMIC_VERSION_MINOR).$(DYNAMIC_VERSION_REVISION)

which is corressponding to setting vesion of caffe binary to same version as library is set to. Which seems like binary of caffe and library got the same version eg. its versioning is consistent. Please explain what Do I miss?

@Nerei
Copy link

Nerei commented Nov 17, 2015

@lukeyeager Delay with my thoughts was due to my vacations. Sorry.

For me hardcoding caffe version in headers is preferable than in build scripts. Most libraries use such way (at least from my own experience).

  • it's single place to increment (now in make and cmake files). Headers are easily parsable (see aforementioned Summary.cmake#L79-L84) to propagate version info to build scripts.
  • client libraries and apps that link/use caffe may implement different behavior and compatibility depending on caffe version definition. Binary distributions without build scripts are possible (say for cluster installs).

@flx42 I find such header useful, and it is generated already and stored in binary folder. See
CMakeLists.txt#L56. But might need to elaborate more, depending on what you want to do with it. Now it is considered as private.

@flx42
Copy link
Contributor

flx42 commented Nov 17, 2015

Yes, I know it's easy to generate this header with autotools/cmake. But it will be more painful with the handwritten Makefile, unfortunately.

I think the use case I described should be a good motivating example, if you don't have a configuration file, using the C++ API can be dangerous when you have ifdefs in the header. It's also easier to check the version.

@lukeyeager
Copy link
Contributor Author

@jczaja

With both build systems, the version reported with the CLI tool or with pycaffe should be MAJOR.MINOR.PATCH, and the SONAME should be MAJOR.MINOR. I believe I'm doing that correctly, though I admit it's less legible in the Makefile.

Are you seeing a bug, or are you just having trouble reading though the code? Either is potentially helpful feedback.

@lukeyeager
Copy link
Contributor Author

@Nerei thanks for the response!

I agree it's relatively easy to parse the header file with CMake. I'll let the caffe devs weigh in on which approach they'd prefer.

@jczaja
Copy link
Contributor

jczaja commented Nov 18, 2015

@lukeyeager

I observed potential issue , so I wanted to let you know about that. So I understand that regardless the build is created using Makefile or CMakeLists.txt , the result in terms of versioning should be the same. Let's consider example that build version is 1.2.0 , and caffe lib version is 1.0.0. Now :
A) CMakeLists.txt scenario:
set (CAFFE_TARGET_VERSION "1.2.0")
set (CAFFE_TARGET_SOVERSION "1.0.0")
add_definitions(-DCAFFE_VERSION=${CAFFE_TARGET_VERSION}) # Here we pass 1.2.0

B) MakeFile
DYNAMIC_VERSION_MAJOR := 1
DYNAMIC_VERSION_MINOR := 0
DYNAMIC_VERSION_REVISION := 0
...
COMMON_FLAGS += -DCAFFE_VERSION=$(DYNAMIC_VERSION_MAJOR).$(DYNAMIC_VERSION_MINOR).$(DYNAMIC_VERSION_REVISION) # Here we pass 1.0.0

So as You can see CAFFE_VERSION when using Makefiles would set to 1.0.0 , and when using CMakeLists.txt CAFFE_VERSION would be set to 1.2.0

Unless I misunderstood something in Makefiles, there is a discrepancy here. Please correct me If necessery

@cbalint13
Copy link
Contributor

Dear All,

My +1 from release engineering point of view:

  • That raw exposed Makefile, in a more proper manner, should be expanded normally from some Makefile.in using old style automake/auotoconf saga (old ways of building on unices). CMake for now do far better job, i believe it will overtake (or should) that Makefile. I also believe nobody will update Makefile to be expanded in proper ways using automake/autoconf tools (its painfull) as it would be standard.
  • Any distro (linux) requires proper so-name versioning on libs as basic rule. Further packaging into .deb, .rpm and so on requires soversion at must level. Once proper so-versioning would be done, will open the path to packagers / distros.
  • As stated, I believe that Makefile should die from a point, nobody will update it. CMake should overtake as primary build configurator / manager. By the way CMake in contrast with autoconf/automake and Make saga enable us to be cross-platform too.

Apologies if my +1 cent intervention looks odd, hope to be in community's service.

With respect,
~Cristian.

@lukeyeager
Copy link
Contributor Author

@jczaja I'm assuming the CAFFE_TARGET_VERSION will always be MAJOR.MINOR.REVISION and the CAFFE_TARGET_SOVERSION will always be MAJOR.MINOR. The VERSION should never be 1.0.0 while the SOVERSION is 1.2.0 because those are different MINOR versions.

This would be equivalent:

# ---[ Caffe version
set(CAFFE_VERSION_MAJOR "1")
set(CAFFE_VERSION_MINOR "0")
set(CAFFE_VERSION_REVISION "0")
set(CAFFE_TARGET_VERSION "${CAFFE_VERSION_MAJOR}.${CAFFE_VERSION_MINOR}.${CAFFE_VERSION_REVISION}")
set(CAFFE_TARGET_SOVERSION "${CAFFE_VERSION_MAJOR}.${CAFFE_VERSION_MINOR}")
add_definitions(-DCAFFE_VERSION=${CAFFE_TARGET_VERSION})

@lukeyeager
Copy link
Contributor Author

@cbalint13 I agree. I've created a new issue at #3351 since it's not necessarily related to this PR.

@lukeyeager
Copy link
Contributor Author

Instead of waiting for 1.0 it seems like it could do good to adopt the new release workflow now for a series of release candidate (RC) versions while we cross off our 1.0 TODOs.
@shelhamer #3409 (comment)

That's fine with me. What would the tag be? I'd suggest v1.0.0-rc.3 since you already have an RC2.

@cdluminate
Copy link
Contributor

Any update to this ?

Currently I'm trying to help debian to update cuda to 7.5, and my local test shows that cuda 7.5 works
well with GCC-5. That means the time we upload the Caffe package gets one step closer.

So do you have any plan on next release of Caffe ?

If the next release date is not far from now, I'd like to wait for the new release and then upload the new release.

Thanks in advance.

@elezar
Copy link
Contributor

elezar commented Jan 15, 2016

Any chance that this could be merged. Would be nice to have version information for #3518.

@shelhamer shelhamer changed the title Mark version 1.0.0 Add library versioning and mark version 1.0.0-rc3 Jan 23, 2016
@lukeyeager lukeyeager deleted the bvlc/versioning branch January 23, 2016 18:24
@Nerei
Copy link

Nerei commented Jan 25, 2016

shelhamer wrote:

Thanks for the versioning @lukeyeager!
p.s. #486 needs a version header for close.

@shelhamer @lukeyeager How about auto generation of caffe_version.h from cmake?
I can implement it by parsing "1.0.0-rc3".

@seanbell
Copy link

Perhaps the commit should be tagged, to make it easier to find releases (e.g. #3595)?

shelhamer added a commit to shelhamer/caffe that referenced this pull request Feb 20, 2016
for linking of the caffe tools, tests, etc. the library install name
needs to include the @rpath set for the executables and interfaces

this was broken on OSX by BVLC#3311
zouxiaochuan pushed a commit to zouxiaochuan/caffe that referenced this pull request Mar 17, 2016
for linking of the caffe tools, tests, etc. the library install name
needs to include the @rpath set for the executables and interfaces

this was broken on OSX by BVLC#3311
SvenTwo pushed a commit to SvenTwo/caffe that referenced this pull request Apr 6, 2016
for linking of the caffe tools, tests, etc. the library install name
needs to include the @rpath set for the executables and interfaces

this was broken on OSX by BVLC#3311
fxbit pushed a commit to Yodigram/caffe that referenced this pull request Sep 1, 2016
for linking of the caffe tools, tests, etc. the library install name
needs to include the @rpath set for the executables and interfaces

this was broken on OSX by BVLC#3311
zouxiaochuan pushed a commit to zouxiaochuan/caffe that referenced this pull request Oct 24, 2016
for linking of the caffe tools, tests, etc. the library install name
needs to include the @rpath set for the executables and interfaces

this was broken on OSX by BVLC#3311
zouxiaochuan pushed a commit to zouxiaochuan/caffe that referenced this pull request Oct 24, 2016
for linking of the caffe tools, tests, etc. the library install name
needs to include the @rpath set for the executables and interfaces

this was broken on OSX by BVLC#3311
zouxiaochuan pushed a commit to zouxiaochuan/caffe that referenced this pull request Feb 15, 2017
for linking of the caffe tools, tests, etc. the library install name
needs to include the @rpath set for the executables and interfaces

this was broken on OSX by BVLC#3311
zouxiaochuan pushed a commit to zouxiaochuan/caffe that referenced this pull request Feb 15, 2017
for linking of the caffe tools, tests, etc. the library install name
needs to include the @rpath set for the executables and interfaces

this was broken on OSX by BVLC#3311
zouxiaochuan pushed a commit to zouxiaochuan/caffe that referenced this pull request Feb 15, 2017
for linking of the caffe tools, tests, etc. the library install name
needs to include the @rpath set for the executables and interfaces

this was broken on OSX by BVLC#3311
zouxiaochuan pushed a commit to zouxiaochuan/caffe that referenced this pull request Feb 15, 2017
for linking of the caffe tools, tests, etc. the library install name
needs to include the @rpath set for the executables and interfaces

this was broken on OSX by BVLC#3311
ssahu pushed a commit to ssahu/caffe that referenced this pull request May 13, 2017
for linking of the caffe tools, tests, etc. the library install name
needs to include the @rpath set for the executables and interfaces

this was broken on OSX by BVLC#3311
acmiyaguchi pushed a commit to acmiyaguchi/caffe that referenced this pull request Nov 13, 2017
for linking of the caffe tools, tests, etc. the library install name
needs to include the @rpath set for the executables and interfaces

this was broken on OSX by BVLC#3311
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.