-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
Conversation
/cc @cdluminate for concerns w.r.t. debian packaging /cc @Nerei for thoughts about the header file approach from #1667 |
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 Thank you for working on versioning it :-) By the way, |
@ronghanghu given your matcaffe contributions do you know how to mark the version in MATLAB? |
Hi, @shelhamer, I think for matlab we could simply do |
Thanks @lukeyeager! This + the simple matcaffe version extension by @ronghanghu looks like all the infrastructure needed to declare a version. |
I think we can merge this PR first, and I will create another PR for matcaffe. |
2812b9c
to
d6472bb
Compare
Rebased after [my] conflicting change from #3127. Any comments on the "for discussion" points above? I agree the matlab versioning can come later. |
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. This file could look like this:
|
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: 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? |
@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).
@flx42 I find such header useful, and it is generated already and stored in binary folder. See |
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. |
With both build systems, the version reported with the CLI tool or with pycaffe should be Are you seeing a bug, or are you just having trouble reading though the code? Either is potentially helpful feedback. |
@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. |
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 : B) MakeFile 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 |
Dear All, My +1 from release engineering point of view:
Apologies if my +1 cent intervention looks odd, hope to be in community's service. With respect, |
@jczaja I'm assuming the 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}) |
@cbalint13 I agree. I've created a new issue at #3351 since it's not necessarily related to this PR. |
That's fine with me. What would the tag be? I'd suggest |
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 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. |
Any chance that this could be merged. Would be nice to have version information for #3518. |
shelhamer wrote:
@shelhamer @lukeyeager How about auto generation of caffe_version.h from cmake? |
Perhaps the commit should be tagged, to make it easier to find releases (e.g. #3595)? |
Close #486, Close #2969, Close #3015, Close #3409
Originally NVIDIA#39 and NVIDIA#46
Included
Adds versioning to:
libcaffe.so
)caffe --version
)caffe.__version__
)TODO
For discussion
libcaffe.MAJOR
instead oflibcaffe.MAJOR.MINOR
.