-
Notifications
You must be signed in to change notification settings - Fork 6k
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
ceph.spec.in:BuildRequires sharutils #4898
Conversation
Oh and its a dependency on suse as well. |
Note this prevents OBS building ceph on fedora. |
IIRC @smithfarm has a pull request in the same area |
@osynge I just poked around a little and it appears that |
@smithfarm the comparison statement in rpm is primitive to say the least. "%{?rhel}" -> "" if no value is set does not help, I suspect best thing to do is remove this as its a dependency catered for with suse in another part of the spec file. @smithfarm yes lets make this a uudecode dependency if that more accurate. |
@osynge in this commit you seem to be saying that |
@smithfarm sure I will reproduce this. Please note that this cannot be shown to fix things without also applying fixes. I will merge all three fixes to one test branch and show you it build on OBS. |
82f0dff
to
6462ca7
Compare
6462ca7
to
458ef32
Compare
Can you please squash the two commits so the build will continue to work during a bisection? |
458ef32
to
6f10abf
Compare
squash the two commits into one. |
@smithfarm we will need a local patch to block obs errors here for fedora. The bug has been reported to them in suse bugzilla. https://build.opensuse.org/project/monitor/home:osynge:ceph:wip:wip_obs_fedora Who should make one? |
the bot failure is a false negative and can be ignored http://tracker.ceph.com/issues/11948 |
LGTM
|
I was in error, now this patch only exposes an OBS bug when java is enabled |
still it is noticeably better than without it. as you can see here https://build.opensuse.org/project/monitor/home:osynge:ceph:wip:wip_obs_fedora |
master+4920+4918+4898 -> work on suse + fedora on OBS allowing them to use OBS for their SUSE work. (This patch is more critical for OBS users working on fedora or verifying they dont break fedora) |
Now with the gitbuilders: http://gitbuilder.sepia.ceph.com/gitbuilder-doc/#origin/wip-4898 |
@ktdreyer This PR passes the gitbuilder test. Do you think it's OK to merge? |
6f10abf
to
df7ee34
Compare
Updated commit message to follow Nathan’s suggestion. |
@@ -54,6 +54,9 @@ Requires(post): binutils | |||
%if 0%{?_with_systemd} | |||
Requires: systemd | |||
%endif | |||
%if 0%{with cephfs_java} | |||
BuildRequires: sharutils |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just realized that we need to use uudecode to decode java bytecode.
looks good to me. |
Pushed to ceph.git as Assuming this builds in gitbuilder, |
Can someone actually describe the nature of the breakage originally motivating this change, and the actual fix proposed? Which build did or didn't get the sharutils dependency, and how did it fail as a result, and how does the proposed fix address this? |
Agreed with @dmick that the commit log could be more informative. For example, would you mind providing the text snipped of the exact build failure in the commit log? |
(and gitbuilder looks good with the latest commit, df7ee34) |
The failure is specific to the Open Build Service http://openbuildservice.org -- here is the snippet from Fedora 20 build without this patch:
What happens is that, for some reason, the OBS fails to install I am guessing that the OBS somehow chokes on the known-buggy conditional currently in master:
which appears to say "if (NOT rhel) OR fedora", which should be true on Fedora. That is how rpmbuild interprets it, but OBS (perhaps due to a bug?) apparently evaluates it as false and does not include |
Thanks @smithfarm . Now that you mention it, I remember that early versions of rpmbuild (in RHEL 4) did not support complex conditionals. It's possible that the rpmbuild version that OBS uses has some trouble with this one. (Or maybe it's something more subtle?) I've confirmed that Koji does pull sharutils into the buildroot when building the code that's currently in master on Fedora, so there must be some difference between OBS and Koji there. At any rate, now that we have with a |
Yes, that and the fact that all the RPM distros we are currently building for have a package called |
The uudecode binary is used to build Java-related components, and uudecode is provided by the sharutils package on all supported RPM platforms. When building with "--without=cephfs_java", sharutils is not needed. Thanks to Nathan Cutler <ncutler@suse.cz> for going into the details with me. On OBS without this patch we get the error message: [ 170s] ----------------------------------------------------------------- [ 170s] ----- building ceph.spec (user abuild) [ 170s] ----------------------------------------------------------------- [ 170s] ----------------------------------------------------------------- [ 170s] + exec rpmbuild -ba --define '_srcdefattr (-,root,root)' --nosignature --define 'disturl obs://build.opensuse.org/home:osynge:ceph:wip:wip_obs_fedora/Fedora_20/c0bbbc1e62228ca956ac3d367edc4fba-master' /home/abuild/rpmbuild/SOURCES/ceph.spec [ 170s] error: Failed build dependencies: [ 170s] sharutils is needed by ceph-1:2+git.1435043747.c1bd02c-1.1.x86_64 With this patch we can build fedora 22 and fedora 20 rpms fine. Signed-off-by: Owen Synge <osynge@suse.com>
df7ee34
to
43c1784
Compare
Ok I changed the git commit msg. Hopefully then this be merged now. |
Thank you for the clarification; that makes much more sense now |
ceph.spec.in:BuildRequires sharutils Reviewed-by: Nathan Cutler <ncutler@suse.com> Reviewed-by: Kefu Chai <kchai@redhat.com> Reviewed-by: Ken Dreyer <kdreyer@redhat.com> Reviewed-by: Dan Mick <dmick@redhat.com>
sharutils rpm is not needed in fedora.
sharutils rpm only a dependency on opensuse.
So this conditional is not needed.
Signed-off-by: Owen Synge osynge@suse.com