Skip to content

Fix gluster master build#150

Closed
amarts wants to merge 1 commit intogluster:masterfrom
amarts:fix-gluster-master-build
Closed

Fix gluster master build#150
amarts wants to merge 1 commit intogluster:masterfrom
amarts:fix-gluster-master-build

Conversation

@amarts
Copy link
Member

@amarts amarts commented Nov 2, 2018

Fixes #75

Also overrides #77 as I had to push one more patch to get the build properly done.

@ghost ghost assigned amarts Nov 2, 2018
@ghost ghost added the in progress label Nov 2, 2018
@amarts amarts requested a review from pkalever November 2, 2018 18:03
@pkalever
Copy link
Contributor

pkalever commented Nov 5, 2018

@amarts @bhumikagoyal IMO, Guster API (gfapi) shouldn't break the backward compatibility.
This will break all the application, such as qemu, tcmu, samba, gluster-block ... and many more.

IMHO, the fix should go in glusterfs.

Thanks!

@lxbsz
Copy link
Collaborator

lxbsz commented Nov 6, 2018

Yeah, agree with Prasanna. And we couldn't assume that only the gluster-block project is using the gfapi.

@pkalever
Copy link
Contributor

pkalever commented Nov 6, 2018

As this is agreed by everyone in the team, probably we should close it.

@amarts feel free to close it accordingly.

@amarts
Copy link
Member Author

amarts commented Nov 6, 2018

As this is agreed by everyone in the team, probably we should close it.

@amarts feel free to close it accordingly.

I agree that the 'ideal' fix is glusterfs not changing anything, but as this project depends on the changes done in glusterfs project, safe-guarding the project with a proper version check is not at all a bad option, rather the right way to go.

@amarts
Copy link
Member Author

amarts commented Nov 6, 2018

As per the discussions in gluster/glusterfs#539 we need to handle the version number properly in gluster-block project itself. Lets continue reviewing and get this going!

The updated gfapi for glusterfs fop ftruncate has pre/post
attributes in the function signature. But this api call
inside gluster-block is not update, so update the
api call for ftruncate to resolve build errors.

Make sure 'config.h' is added in build properly

Notice that, this is required for building gluster-block
with glusterfs 'master' branch. As glusterfs glfs-api signature
and the development header path are being modified in master,
there may be more things in plate before next version release,
and hence keeping the flag as 'NOT_NEW' for current stack.

Fixes: gluster#75
Signed-off-by: Bhumika Goyal <bgoyal@redhat.com>
Signed-off-by: Amar Tumballi <amarts@redhat.com>
@amarts amarts force-pushed the fix-gluster-master-build branch from 8aa3e6e to 2a780a1 Compare December 17, 2018 18:45
@amarts
Copy link
Member Author

amarts commented Dec 17, 2018

@pkalever @lxbsz please take a look. This is important for keeping the project up-to-date with glusterfs-master branch, and there may be more changes. But as it doesn't break anything in existing code, I would prefer to have it.

If you notice, without this, we are building the project with glusterfs-5.1.x right now in our nightly builds here: https://ci.centos.org/view/Gluster/job/gluster_block_basic/21/console. Ideal is to build it everyday with glusterfs master from previous night. With this patch, we can achieve that.

@lxbsz
Copy link
Collaborator

lxbsz commented Dec 20, 2018

@pkalever @lxbsz please take a look. This is important for keeping the project up-to-date with glusterfs-master branch, and there may be more changes. But as it doesn't break anything in existing code, I would prefer to have it.

If you notice, without this, we are building the project with glusterfs-5.1.x right now in our nightly builds here: https://ci.centos.org/view/Gluster/job/gluster_block_basic/21/console. Ideal is to build it everyday with glusterfs master from previous night. With this patch, we can achieve that.

Is it possible that in future the API will be updated again, like this ?

If so IMO it will be better that to use a macro that has the version number instead of GLUSTERFS_NOT_NEW, something like GLUSTERFS_VERSION_5_1_X. Will this make sense here ?

Thanks.

@amarts
Copy link
Member Author

amarts commented Dec 20, 2018

something like GLUSTERFS_VERSION_5_1_X. Will this make sense here ?

Agree! We can do something like that.

@pkalever
Copy link
Contributor

Fixed by: #178

@pkalever pkalever closed this Mar 25, 2019
@ghost ghost removed the in progress label Mar 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants