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

UPDATED: doc (ABI comparison) and various other fixes #324

Merged
merged 13 commits into from
Sep 25, 2018

Conversation

chrissie-c
Copy link
Contributor

This is an updated version on Poki's PR#287. It fixes 3 things

  1. Updated to latest tests/Makefile.am
  2. Compatibility with check 0.9 (uints were introduced in 0.10)
  3. Fixed C++ compiler check

4aa9d08 can be ignored as it's reverted by cf6c346. I'll squash them on commit.

jnpkrn and others added 12 commits January 4, 2018 14:50
(It made a service as-was, but being afforded more time, this would
have accompanied that commit right away, for better understanding,
brevity and uniformity.)

Signed-off-by: Jan Pokorný <[email protected]>
There was a significant redundancy wrt. build flags and EXTRA_DIST
assignment (the latter become redundant as of f6e4042 at latest)
spread all over the place (vivat copy&paste).  Also, in one instance,
CPPFLAGS (used) was confused with CFLAGS (meant).

Signed-off-by: Jan Pokorný <[email protected]>
1. ABICC >= 2 needs to be passed -cxx-incompatible switch because C is
   no longer a default for this tool (used to be vice versa),
   plus current version will stop choking on C vs. C++ (our C code with
   C++ compatibility wrapping being viewed from C++ perspective for the
   purpose of dumping the declared symbols, which somewhat conflicts
   with internal masking of the C++ keywords being used as valid C
   identifiers [yet some instances must not be masked here, see
   lvc/abi-compliance-checker#64) only
  if _also_ something like this is applied:
   lvc/abi-compliance-checker#70

2. since 20246f5, libqb.so no longer poses a symlink to the actual
   version-qualified shared library, but rather a standalone linker
   script, which confuses ABICC, so blacklist that file for the scanning
   purposes explicitly, together with referring to the library through
   it's basic version qualification (which alone, sadly, is not
   sufficient as ABICC proceeds to scan whole containing directory
   despite particular file is specified)

Signed-off-by: Jan Pokorný <[email protected]>
Beside avoiding issues with abi-compliance-checker in the role of ABI
dumps producer (see the preceding commit), it also seems to generate
more accurate picture (maybe because it expressly requires compiling
with debugging information requested).

Signed-off-by: Jan Pokorný <[email protected]>
Primary trigger was the fact that "./check abi" (via
build-aux/generate-docs) choked on that because abi-compliance-checker
(ABICC) uses g++ to list symbols in header files playing hide-and-see
regarding C identifiers otherwise conflicting with C++ keywords,
which is moreover triggered only with -cxx-incompatible switch
since ABICC 2.0 (and even then the respective code needs some further
tweaking: lvc/abi-compliance-checker#70),
and this switch wasn't historically applied.

Anyway, this C++ perspective there was likely unprecedented even
though libqb no doubt targets such compatiblity.  To avoid its
breakage within the public headers' scope in the future, arrange
for checking it regularly alongside the "auto_check_header" tests
(cf. make -C tests check-headers).

Signed-off-by: Jan Pokorný <[email protected]>
IIRC, Chrissie asked about this around inclusion of the test at
hand, and it seemed there was no way but to emit a warning to get
something output at all.  Now it turns wrong, and moreover, we
make the code not fixed on GCC specific pragmas, with a bit of
luck, "#pragma message" approach is adopted more widely by compilers.

Signed-off-by: Jan Pokorný <[email protected]>
it's not available in check 0.9 which is in RHEL7
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2016 Red Hat, Inc.
* Copyright 2018 Red Hat, Inc.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not want to contribute into meaningless discussion, but this really should be Copyright (C) 2016-2018, as an example see /usr/include/*.h (for example math.h)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost right :) It could be either Copyright 2016,2018 or Copyright 2016-2018. The (C) convention, while common, does not conform to copyright regulations.

Copy link
Member

@jfriesse jfriesse Sep 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm nether lawyer nor native English speaker, so I don't want to argue (my comment was simply based on lgpl how-to-apply + keep consistency with rest of libqb), but just want to ask. I can fully understand that (c)/(C) is duplicate of Copyright, so formally doesn't need to be there. But you are talking about "does not conform to (c) regulations", so does it mean "Copyright (c)" makes license incorrect? Does it also mean https://www.gnu.org/licenses/lgpl-2.0.txt section "How to Apply These Terms to Your New Libraries" is incorrect (this applies also for newer licenses like GPL-3 - https://www.gnu.org/licenses/gpl-3.0.txt)? If so, shouldn't we change this everywhere (and I'm not talking just about libqb, but also about corosync/qdevice/pacemaker (some files has a (c) some files doesn't))?

Copy link
Contributor

@kgaillot kgaillot Sep 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a big deal, because copyright notices seem to have little practical significance other than defining who has to be contacted if the license must be changed. However, U.S. copyright regulations are clear (treaties ensure that copyright law is similar across most countries, but I am not sure if this is covered by that); a copyright notice contains:

The symbol © (the letter C in a circle), or the word "Copyright" or the abbreviation "Copr."; 

https://www.bitlaw.com/copyright/formalities.html

followed immediately by the year and the copyright holder.

The obvious intent of "(c)" or "(C)" is to replace the c-in-a-circle copyright symbol with ASCII, but the law does not recognize that as equivalent, and even the symbol is unnecessary anyway when Copyright is spelled out.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thank you a lot for explanation (and confirmation that law and common sense are not always in agreement :) )

Copy link
Contributor

@kgaillot kgaillot Sep 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, and you are correct that the original year is the most important one to put in the copyright notice. The purpose of a copyright notice is to establish a claim to priority (in the sense of "which came prior") and the date at which copyright protections expire.

Copyright has long had to deal with publications that are updated, such as encyclopedias, so there is some established usage there, though it is not specified by law: either a range like 2014-2018 or a list of specific years like 2014,2015,2018.

https://techwhirl.com/updating-copyright-notices/

The legal side hasn't kept up with the idea of projects updated by many contributing copyright holders, so the law does not specify a format for that, and it is only convention. I've seen multiple variations; some list only the original copyright holder, some put a separate notice for modifications by a later author, some put the original holder "and the $PROJECT project contributors".

It's primarily an intellectual exercise though, as the chances of someone bringing legal action for copyright issues in libqb is vanishingly small, and the chance that the copyright notice line would have any significant bearing on that is even more infinitesimal.

@@ -0,0 +1,107 @@
/*
* Copyright 2018 Red Hat, Inc.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(C) missing

@chrissie-c
Copy link
Contributor Author

I think I'd rather go for consistency and leave the (c) in - thanks for spotting that. AFAIR when we asked for legal advice on this, the response what that it actually doesn't much matter what we put in that line from their point of view.

@jfriesse
Copy link
Member

ACK by me if "(c)" (year one) issue is fixed.

I understand this is not legally needed but at least it makes the files
consistent.
@chrissie-c chrissie-c merged commit d0ec0a6 into master Sep 25, 2018
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