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

Btrfs has eliminated the BTRFS_BUILD_VERSION in latest version #11417

Merged
merged 2 commits into from Mar 24, 2015

Conversation

rhatdan
Copy link
Contributor

@rhatdan rhatdan commented Mar 16, 2015

They say we should only use the BTRFS_LIB_VERSION

They will no longer support this since it had to be managed manually

Docker-DCO-1.1-Signed-off-by: Dan Walsh dwalsh@redhat.com (github: rhatdan)

@tianon
Copy link
Member

tianon commented Mar 16, 2015

@rhatdan
Copy link
Contributor Author

rhatdan commented Mar 16, 2015

The comments in the bugzilla seemed to indicate that upstream would not take a patch to put the BTRFS_BUILD_VERSION back since it was being manually created while BTRFS_LIB_VERSION can be automatically created.

@jessfraz
Copy link
Contributor

# github.com/docker/docker/daemon/graphdriver/btrfs
./version_test.go:10: invalid argument BtrfsLibVersion() (type int) for len
Precompiled: github.com/docker/docker/daemon/graphdriver/overlay
Precompiled: github.com/docker/docker/daemon
Precompiled: github.com/docker/docker/daemon/networkdriver
Precompiled: github.com/docker/docker/daemon/graphdriver/devmapper
Precompiled: github.com/docker/docker/daemon/graphdriver/vfs
Precompiled: github.com/docker/docker/daemon/networkdriver/ipallocator
Precompiled: github.com/docker/docker/daemon/networkdriver/portallocator
Precompiled: github.com/docker/docker/daemon/networkdriver/portmapper
Precompiled: github.com/docker/docker/daemon/networkdriver/bridge

@lsm5
Copy link
Contributor

lsm5 commented Mar 16, 2015

@tianon @rhatdan for now, sandeen and kdave sound ok with it: https://bugzilla.redhat.com/show_bug.cgi?id=1202373#c4 and given that the patch grabs whatever value exists for PACKAGE_VERSION from its config.h, it shouldn't be a hassle I think. correct me if i'm wrong.

@jessfraz
Copy link
Contributor

can you fix the unit test @rhatdan, otherwise this seems like a sane fix to me :)

@rhatdan
Copy link
Contributor Author

rhatdan commented Mar 17, 2015

Fixed.

@prologic
Copy link
Contributor

/cc @prologic

They say we should only use the BTRFS_LIB_VERSION

They will no longer support this since it had to be managed manually

Docker-DCO-1.1-Signed-off-by: Dan Walsh <dwalsh@redhat.com> (github: rhatdan)
@jessfraz
Copy link
Contributor

still failing the unit tests, would be nice to get this into 1.6.0-rc2, do you want us to carry

@jessfraz jessfraz added this to the 1.6.0 milestone Mar 24, 2015
@vbatts
Copy link
Contributor

vbatts commented Mar 24, 2015

@jfrazelle strange. and the build box has the have /usr/include/btrfs/version.h?

+ go test -test.timeout=45m github.com/docker/docker/daemon/graphdriver/btrfs
--- FAIL: TestLibVersion (0.00s)
    version_test.go:11: expected output from btrfs lib version > 0
FAIL

would only happen if docker was built with btrfs_noversion

@vbatts
Copy link
Contributor

vbatts commented Mar 24, 2015

OH! ubuntu ... :-(
it is built with btrfs_noversion

@jessfraz
Copy link
Contributor

oh so wait should i just upgrade btrfs-tools is that the problem

On Tue, Mar 24, 2015 at 10:25 AM, Vincent Batts notifications@github.com
wrote:

OH! ubuntu ... :-(
it is built with btrfs_noversion


Reply to this email directly or view it on GitHub
#11417 (comment).

@vbatts
Copy link
Contributor

vbatts commented Mar 24, 2015

well, ideally the test would be on a newer btrfs... but would mean the test will fail when there is no version.h.

@rhatdan perhaps move you Lib Version test to a _test.go file that has the build tags of // +build linux,btrfs_noversion

Docker-DCO-1.1-Signed-off-by: Dan Walsh <dwalsh@redhat.com> (github: rhatdan)
@rhatdan
Copy link
Contributor Author

rhatdan commented Mar 24, 2015

Sadly I did that locally but never pushed.

@vbatts
Copy link
Contributor

vbatts commented Mar 24, 2015

https://www.youtube.com/watch?v=vCadcBR95oU

On Tue, Mar 24, 2015 at 2:15 PM, Daniel J Walsh notifications@github.com
wrote:

Sadly I did that locally but never pushed.


Reply to this email directly or view it on GitHub
#11417 (comment).

@vbatts
Copy link
Contributor

vbatts commented Mar 24, 2015

\o/

@vbatts
Copy link
Contributor

vbatts commented Mar 24, 2015

LGTM

@tianon
Copy link
Member

tianon commented Mar 24, 2015

Aw, that's a shame. The library version is completely 1000% meaningless to me (where the "build version" actually matches the version number of the package I've got installed), but OK.

LGTM

vbatts added a commit that referenced this pull request Mar 24, 2015
Btrfs has eliminated the BTRFS_BUILD_VERSION in latest version
@vbatts vbatts merged commit 8fc9e40 into moby:master Mar 24, 2015
@tianon
Copy link
Member

tianon commented Mar 24, 2015

Couldn't we instead find a way to conditionally include this based on that one release of btrfs-progs that doesn't have the build version info (since they added it back thanks to @lsm5)?

@tianon
Copy link
Member

tianon commented Mar 24, 2015

It's especially worrying since they never responded to https://bugzilla.redhat.com/show_bug.cgi?id=1202373#c5 to tell us whether they approve of our use of this header bit or not. 😢

@vbatts
Copy link
Contributor

vbatts commented Mar 24, 2015

@tianon an #ifdef check would be easy enough, and the stubbing out that info in the Status(). That's what I'd started. Perhaps by that time, we can have a better position from upstream.

@jessfraz
Copy link
Contributor

/me is not pleased there is a merge commit tsk tsk

@tianon
Copy link
Member

tianon commented Mar 24, 2015 via email

@vbatts
Copy link
Contributor

vbatts commented Mar 24, 2015

Meaningless, but more than nothing. I also understand the difficulty in
maintaining a string like that, when it is not automatically produced. When
/if they add a better version string, we'll consume it.
On Mar 24, 2015 4:12 PM, "Tianon Gravi" notifications@github.com wrote:

@vbatts 👍 😍


Reply to this email directly or view it on GitHub
#11417 (comment).

@tianon
Copy link
Member

tianon commented Mar 24, 2015 via email

jessfraz added a commit to jessfraz/docker that referenced this pull request Mar 25, 2015
…diff-479b910834cf0e4daea2e02767fd5dc9R1 pr moby#11417

Docker-DCO-1.1-Signed-off-by: Jessica Frazelle <jess@docker.com> (github: jfrazelle)
jessfraz added a commit to jessfraz/docker that referenced this pull request Mar 25, 2015
…diff-479b910834cf0e4daea2e02767fd5dc9R1 pr moby#11417

Docker-DCO-1.1-Signed-off-by: Jessica Frazelle <jess@docker.com> (github: jfrazelle)
@vbatts
Copy link
Contributor

vbatts commented Mar 25, 2015

@lsm5
Copy link
Contributor

lsm5 commented Mar 26, 2015

/me is late to the party... the BUILD_VERSION patch has now landed in btrfs-progs master branch

@jessfraz
Copy link
Contributor

yayyyyy!

On Wed, Mar 25, 2015 at 5:00 PM, Lokesh Mandvekar notifications@github.com
wrote:

/me is late to the party... the BUILD_VERSION patch has now landed in
btrfs-progs master branch


Reply to this email directly or view it on GitHub
#11417 (comment).

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.

None yet

7 participants