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

build/ops: miscellaneous cleanups and fixes (run-make-check.sh, ceph.spec.in) #15399

Merged
merged 6 commits into from Jun 14, 2017

Conversation

Projects
None yet
5 participants
@smithfarm
Contributor

smithfarm commented May 31, 2017

@smithfarm smithfarm changed the title from build/ops: make sure which is installed in run-make-check.sh to build/ops: make sure which is installed in run-make-check.sh and fix RPMLINT warnings May 31, 2017

@smithfarm smithfarm requested review from tchaikov and ktdreyer Jun 1, 2017

@ktdreyer

This comment has been minimized.

Member

ktdreyer commented Jun 1, 2017

What's the exact rpmlint error you're getting for %defattr? This should be the default for years in RPM. Could it be a bug in rpmlint?

@ktdreyer

This comment has been minimized.

Member

ktdreyer commented Jun 1, 2017

Whoops, sorry, I see it in your Git commit log. It was removed from rpmlint in rpm-software-management/rpmlint#56

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jun 1, 2017

@ktdreyer So what you're saying is I could drop this line from all the %files sections ;-)

@ktdreyer

This comment has been minimized.

Member

ktdreyer commented Jun 1, 2017

Yep!

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jun 2, 2017

@smithfarm smithfarm changed the title from build/ops: make sure which is installed in run-make-check.sh and fix RPMLINT warnings to build/ops: reflect that "which" is needed to run "make check" and drop redundant defattrs from spec file Jun 2, 2017

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jun 2, 2017

Shaman is green.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jun 2, 2017

Changelog:

  • made the run-make-check.sh patch work for Debian/Ubuntu as well as CentOS/SUSE
  • reworked the last patch to drop all redundant defattr lines instead of adding more

@smithfarm smithfarm changed the title from build/ops: reflect that "which" is needed to run "make check" and drop redundant defattrs from spec file to build/ops: miscellaneous cleanups and fixes (run-make-check.sh, ceph.spec.in) Jun 3, 2017

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jun 3, 2017

Changelog:

  • added two more commits addressing minor issues in the spec file
@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jun 3, 2017

Hum. On CentOS 7.3 I get:

[ubuntu@target137074024216 ceph]$ ./run-make-check.sh
Checking hostname sanity... OK
Loaded plugins: fastestmirror
Loading mirror speeds from cached hostfile
 * base: mirror.neify.es
 * extras: centos.crazyfrogs.org
 * updates: mirror.neify.es
No package ccache available.
No package jq available.
No package debianutils available.
Error: Nothing to do
@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jun 3, 2017

Changelog:

  • fixed stupid mistake in the first commit

@smithfarm smithfarm requested a review from b-ranto Jun 3, 2017

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jun 3, 2017

@b-ranto @ktdreyer @tchaikov Tested on Ubuntu and CentOS that it doesn't break run-make-check.sh. Ready for review.

@smithfarm

This comment has been minimized.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jun 5, 2017

Passes shaman

@@ -82,7 +82,7 @@ Source0: http://ceph.com/download/@TARBALL_BASENAME@.tar.bz2
%if 0%{?is_opensuse}
ExclusiveArch: x86_64 aarch64 ppc64 ppc64le
%else
ExclusiveArch: x86_64 aarch64
ExclusiveArch: x86_64 aarch64 ppc64le s390x

This comment has been minimized.

@tchaikov

tchaikov Jun 5, 2017

Contributor

the title of this PR puts

Enable ppc64le and s390x builds in SLE

but it appears that we are disabling them.

This comment has been minimized.

@smithfarm

smithfarm Jun 5, 2017

Contributor

@tchaikov: ExclusiveArch is different from ExcludeArch

This comment has been minimized.

@smithfarm

smithfarm Jun 5, 2017

Contributor

it means "exclude all archs other than these"

This comment has been minimized.

@tchaikov

tchaikov Jun 5, 2017

Contributor

ahh, right!

This comment has been minimized.

@b-ranto

b-ranto Jun 5, 2017

Contributor

It is enabling it on suse that is not opensuse which is SLE I suppose,

edit: hmm, too late

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jun 5, 2017

Thanks @tchaikov for the review

@@ -72,7 +72,7 @@ Epoch: 1
%global _epoch_prefix %{?epoch:%{epoch}:}
Summary: User space components of the Ceph file system
License: LGPL-2.1 and CC-BY-SA-1.0 and GPL-2.0 and BSL-1.0 and GPL-2.0-with-autoconf-exception and BSD-3-Clause and MIT
License: LGPL-2.1 and CC-BY-SA-1.0 and GPL-2.0 and BSL-1.0 and GPL-2.0 WITH Autoconf-exception-2.0 and BSD-3-Clause and MIT

This comment has been minimized.

@b-ranto

b-ranto Jun 5, 2017

Contributor

Just a random thought: we no longer use autoconf, do we still need the exception bit?

This comment has been minimized.

@smithfarm

smithfarm Jun 5, 2017

Contributor

@b-ranto Good catch! Updating . . .

@@ -82,7 +82,7 @@ Source0: http://ceph.com/download/@TARBALL_BASENAME@.tar.bz2
%if 0%{?is_opensuse}
ExclusiveArch: x86_64 aarch64 ppc64 ppc64le
%else
ExclusiveArch: x86_64 aarch64
ExclusiveArch: x86_64 aarch64 ppc64le s390x

This comment has been minimized.

@b-ranto

b-ranto Jun 5, 2017

Contributor

It is enabling it on suse that is not opensuse which is SLE I suppose,

edit: hmm, too late

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jun 5, 2017

Changelog:

  • drop "GPL 2.0 with Autoconf Exception" license completely instead of just tweaking spec file to make it pass SPDX 2.0
@b-ranto

b-ranto approved these changes Jun 5, 2017

@@ -134,6 +134,9 @@ BuildRequires: snappy-devel
BuildRequires: udev
BuildRequires: util-linux
BuildRequires: valgrind-devel
%if %{with make_check}
BuildRequires: which
%endif

This comment has been minimized.

@smithfarm

smithfarm Jun 6, 2017

Contributor

@b-ranto @tchaikov I'm tempted to remove the conditional here and just build-require which, even though strictly speaking it's only needed for the tests. What do you think?

I'm not sure how to pass --with make_check to rpmspec in install-deps.sh and it seems like it's not even worth the effort.

(Background: we discovered this issue by running "make check" in a rather pristine Cloud environment - it's rare to find a "real" machine that doesn't have which installed)

This comment has been minimized.

@b-ranto

b-ranto Jun 6, 2017

Contributor

Honestly, I would probably be happiest if we got rid of which altogether and used type -P instead. It is part of bash and is even more safe as it does a direct lookup in PATH (is not alias-aware, etc).

This comment has been minimized.

@tchaikov

tchaikov Jun 6, 2017

Contributor

yeah, type -P is better. but that'd need more work in src/test i guess.

This comment has been minimized.

@b-ranto

b-ranto Jun 6, 2017

Contributor

Yeah, I looked at the sources and it would not be as easy as I have hoped for.

btw: I would be ok if there was no condition around it.

This comment has been minimized.

@tchaikov

tchaikov Jun 6, 2017

Contributor

@smithfarm i am fine also.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jun 6, 2017

Changelog:

  • made which build dependency unconditional (updated commit message)
@smithfarm

This comment has been minimized.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jun 8, 2017

Pushed the latest SHA1 to shaman. Assuming it builds and nobody disagrees, I'd say this is ready to merge.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jun 8, 2017

Shaman is green

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jun 8, 2017

Changelog:

  • it came to my attention that the ceph-mgr subpackage has its own License line (why??) so I had to remove the autoconf exception license there, too

@ktdreyer Do you know if there's a reason why ceph-mgr needs a separate license line?

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jun 8, 2017

Changelog:

  • added a commit dropping all the (unmaintained, buggy) subpackage License lines
@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jun 8, 2017

Jenkins re-test this please

@smithfarm

This comment has been minimized.

@ktdreyer

This comment has been minimized.

Member

ktdreyer commented Jun 8, 2017

Do you know if there's a reason why ceph-mgr needs a separate license line?

Sounds like a bad copy-and-paste?

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jun 8, 2017

@tserong Was there a legal reason why you gave ceph-mgr a separate License line in 2e9e21e ?

@tserong

This comment has been minimized.

Contributor

tserong commented Jun 9, 2017

Was there a legal reason why you gave ceph-mgr a separate License line in 2e9e21e ?

@smithfarm, no, not that I recall. Given that it's identical to the License line on the main package, I can only suggest that it was a redundant copy and paste :-/ I'm happy for it to be removed.

@@ -403,7 +402,6 @@ Summary: OCF-compliant resource agents for Ceph daemons
%if 0%{?suse_version}
Group: System/Filesystems
%endif
License: LGPL-2.0

This comment has been minimized.

@tchaikov

tchaikov Jun 9, 2017

Contributor

this should be just LGPL-2.1, i guess?

This comment has been minimized.

@tchaikov

tchaikov Jun 9, 2017

Contributor

yes, a single license would be easier. but if a subpackage can be licensed only under one license (for example, LGPL 2.1), shall reuse the LGPL-2.1 and CC-BY-SA-1.0 and GPL-2.0 and BSL-1.0 and BSD-3-Clause and MIT license?

This comment has been minimized.

@smithfarm

smithfarm Jun 9, 2017

Contributor

@tchaikov How do we tell what license applies to a given subpackage? I'd like to keep this as low-maintenance as possible.

This comment has been minimized.

@smithfarm

smithfarm Jun 9, 2017

Contributor

s/what license/which license(s)/

This comment has been minimized.

@smithfarm

smithfarm Jun 9, 2017

Contributor

Given that neither the Fedora nor the SUSE packaging guidelines require subpackage license lines, why have them?

This comment has been minimized.

@smithfarm

smithfarm Jun 9, 2017

Contributor

The key words here are "may differ . . . if necessary"

If we are going to declare a separate/difference license for a subpackage, there would need to be some justification why it is "necessary".

This comment has been minimized.

@smithfarm

smithfarm Jun 9, 2017

Contributor

but if a subpackage can be licensed only under one license (for example, LGPL 2.1), shall reuse the LGPL-2.1 and CC-BY-SA-1.0 and GPL-2.0 and BSL-1.0 and BSD-3-Clause and MIT license?

Two thoughts. First, I understand that main license line to mean "Whichever of the following licenses is applicable: LGPL-2.1, CC-BY-SA-1.0, GPL-2.0, BSL-1.0, BSD-3-Clause, MIT". Second, I think it will be easier to maintain if we include separate subpackage license declarations only where there is some explicit legal reason to do so.

This comment has been minimized.

@tchaikov

tchaikov Jun 9, 2017

Contributor

How do we tell what license applies to a given subpackage? I'd like to keep this as low-maintenance as possible.

sorry, no short answer. it depends on what that package is involved with, and its compile time and runt-time dependencies. for example, if it is a derivative work of GPL licensed software. it's GPL'ed also.

If we are going to declare a separate/difference license for a subpackage, there would need to be some justification why it is "necessary".

good q. i am just not sure why we have separated licenses for the subpackages in the very beginning. did we add them because it's "necessary"? if not, it'd be great to use a single license.

This comment has been minimized.

@smithfarm

smithfarm Jun 9, 2017

Contributor

i am just not sure why we have separated licenses for the subpackages in the very beginning. did we add them because it's "necessary"? if not, it'd be great to use a single license.

I did some digging, and in the beginning (2010, 2011) the main package had the line "License: LGPLv2" (later changed to "LGPL-2.0"). Later, as the individual subpackages were added, this line was always copy-pasted. I doubt anyone put much thought into it.

This comment has been minimized.

@tchaikov

tchaikov Jun 9, 2017

Contributor

okay, i guess probably we can use a single license at the moment until someone justifies that it's necessary to license a subpackage with a difference license. but since IANAL and i am far from an expert in this area, so cannot be 100% sure about this change.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jun 9, 2017

Changelog:

  • drop autoconf license from debian/copyright as well
@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jun 9, 2017

Jenkins re-test this please

1 similar comment
@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jun 10, 2017

Jenkins re-test this please

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jun 10, 2017

@tchaikov Something happened with Jenkins, so I can't merge this.

smithfarm added some commits Jan 12, 2017

build/ops: rpm: Enable ppc64le and s390x builds in SLE
Signed-off-by: Nathan Cutler <ncutler@suse.com>
build/ops: rpm, COPYING: drop GPL 2.0 with Autoconf Exception
This license is no longer used.

Fixes: http://tracker.ceph.com/issues/20091
Signed-off-by: Nathan Cutler <ncutler@suse.com>
build/ops: rpm: add which as a build dependency, unconditionally
Although "which" is only used in the tests, we need the dependency
to be picked up by install-deps.sh.

This is a stopgap measure until we can get rid of the dependency entirely by
replacing "which" with "type -P" in the tests.

Fixes: http://tracker.ceph.com/issues/20127
Signed-off-by: Nathan Cutler <ncutler@suse.com>
build/ops: rpm: ceph.spec.in: drop redundant defattrs
The defattr(-,root,root,-) has been the default for a long time.

Signed-off-by: Nathan Cutler <ncutler@suse.com>
build/ops: rpm: one License line to rule them all
The SUSE and Fedora packaging guidelines specify that subpackages _may_ have
their own License line if it is necessary. (Hopefully it's not, because
maintaining one License line is much easier.)

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

@smithfarm smithfarm merged commit 85b0b8a into ceph:master Jun 14, 2017

3 of 4 checks passed

arm64 make check arm64 make check started
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details

@smithfarm smithfarm deleted the smithfarm:wip-20127 branch Jun 14, 2017

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jun 14, 2017

rebased, merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment