[go: nahoru, domu]

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

ceph.spec.in:BuildRequires sharutils #4898

Merged
merged 1 commit into from
Jun 24, 2015

Conversation

osynge
Copy link
@osynge osynge commented Jun 8, 2015

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

@osynge
Copy link
Author
osynge commented Jun 8, 2015

Oh and its a dependency on suse as well.

@osynge
Copy link
Author
osynge commented Jun 8, 2015

Note this prevents OBS building ceph on fedora.

@ghost
Copy link
ghost commented Jun 8, 2015

IIRC @smithfarm has a pull request in the same area

@ghost ghost assigned smithfarm Jun 8, 2015
@smithfarm
Copy link
Contributor

@osynge sorry, when I opened #4881 I didn't realize you were working on the same thing - I have a hunch that the problem was caused by the ! in %if ! 0%{?rhel} || 0%{?fedora} which caused fedora to not get sharutils - does that sound reasonable?

@smithfarm
Copy link
Contributor

@osynge I just poked around a little and it appears that sharutils is a BuildRequires because it contains uudecode which is used in the build process: see output of grep -r uudecode . in master git checkout. This would indicate that it is needed in all RPM-based distros and should probably be moved up into the common section. Do you agree?

@osynge
Copy link
Author
osynge commented Jun 8, 2015

@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.

@smithfarm
Copy link
Contributor

@osynge in this commit you seem to be saying that BuildRequires: sharutils is (1) not needed in Fedora and (2) it's presence actually breaks the Fedora build. Can you reproduce the OBS build failure after rebasing this commit to current master? I would be curious to read the build log.

@osynge
Copy link
Author
osynge commented Jun 9, 2015

@smithfarm sure I will reproduce this. Please note that this cannot be shown to fix things without also applying fixes.

#4906
#4897

I will merge all three fixes to one test branch and show you it build on OBS.

@ktdreyer
Copy link
Member
ktdreyer commented Jun 9, 2015

Can you please squash the two commits so the build will continue to work during a bisection?

@osynge
Copy link
Author
osynge commented Jun 9, 2015

squash the two commits into one.

@osynge
Copy link
Author
osynge commented Jun 9, 2015

@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?

@ghost
Copy link
ghost commented Jun 10, 2015

the bot failure is a false negative and can be ignored http://tracker.ceph.com/issues/11948

@smithfarm
Copy link
Contributor

LGTM

Reviewed-by: Nathan Cutler <ncutler@suse.com>

@smithfarm smithfarm assigned ktdreyer and unassigned smithfarm Jun 10, 2015
@osynge
Copy link
Author
osynge commented Jun 10, 2015

I was in error, now this patch only exposes an OBS bug when java is enabled

@osynge
Copy link
Author
osynge commented Jun 11, 2015

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

@osynge
Copy link
Author
osynge commented Jun 12, 2015

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)

@smithfarm
Copy link
Contributor

@smithfarm
Copy link
Contributor

@ktdreyer This PR passes the gitbuilder test. Do you think it's OK to merge?

@osynge
Copy link
Author
osynge commented Jun 16, 2015

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
Copy link
Contributor

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.

@tchaikov
Copy link
Contributor

looks good to me.

@ktdreyer
Copy link
Member

Pushed to ceph.git as wip_ceph_spec_sharutils so it runs through gitbuilder

Assuming this builds in gitbuilder, Reviewed-by: Ken Dreyer <kdreyer@redhat.com>

@dmick
Copy link
Member
dmick commented Jun 23, 2015

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?

@ktdreyer
Copy link
Member

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?

@ktdreyer
Copy link
Member

(and gitbuilder looks good with the latest commit, df7ee34)

@smithfarm
Copy link
Contributor

The failure is specific to the Open Build Service http://openbuildservice.org -- here is the snippet from Fedora 20 build without this patch:

[  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

What happens is that, for some reason, the OBS fails to install sharutils in the build environment and hence rpmbuild fails immediately on the missing build dependency.

I am guessing that the OBS somehow chokes on the known-buggy conditional currently in master:

%if ! 0%{?rhel} || 0%{?fedora}

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 sharutils in the build environment.

@ktdreyer
Copy link
Member

That is how rpmbuild interprets it, but OBS (perhaps due to a bug?) apparently evaluates it as false and does not include sharutils in the build environment.

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 with cephfs_java conditional to utilize, this looks right to me.

@smithfarm
Copy link
Contributor

At any rate, now that we have with a with cephfs_java conditional to utilize, this looks right to me.

Yes, that and the fact that all the RPM distros we are currently building for have a package called sharutils containing the uudecode binary.

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>
@osynge
Copy link
Author
osynge commented Jun 24, 2015

Ok I changed the git commit msg. Hopefully then this be merged now.

@dmick
Copy link
Member
dmick commented Jun 24, 2015

Thank you for the clarification; that makes much more sense now

@ktdreyer ktdreyer merged commit 43c1784 into ceph:master Jun 24, 2015
ktdreyer added a commit that referenced this pull request Jun 24, 2015
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>
@smithfarm smithfarm deleted the wip_ceph_spec_sharutils branch September 3, 2015 12:54
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.

7 participants