search

openwall / passwdqc

Password/passphrase strength checking and policy enforcement
https://www.openwall.com/passwdqc/
Other
40 stars 17 forks source link

Add github CI #9

Closed ldv-alt closed 3 years ago

ldv-alt commented 3 years ago

This enables a whitespace check and a bunch of build checks using several versions of gcc and clang.

solardiz commented 3 years ago

Thank you, @ldv-alt! I've just merged this as-is, but I have these minor comments:

  1. Some files added for CI say they're under GPL, which is inconsistent with what our LICENSE says:
The rest of the files in this package fall under the following terms
(heavily cut-down "BSD license"):

Redistribution and use in source and binary forms, with or without
modification, are permitted.

Perhaps this inconsistency is now for you to address in one way or another?

  1. The clang warning you mention was also seen with some versions of gcc (some 4.x and 5.x) and with MSVC, possibly more. I wonder if this same fix will work for those - somehow I doubt it, and we might need to use an #if instead (if we care to fix this). I'll test with those gcc soon (don't intend to do another Windows build soon, so won't test there soon).

  2. I think I'll simply drop my e-mail like signature from README.

(More importantly, I think our README is out of date in that it focuses on the PAM module only - I think under that file name this documentation file is supposed to describe the entire package - but this is indeed more work, not just a quick fix.)

ldv-alt commented 3 years ago

On Fri, Mar 12, 2021 at 04:30:14AM -0800, Solar Designer wrote:

Thank you, @ldv-alt! I've just merged this as-is, but I have these minor comments:

  1. Some files added for CI say they're under GPL, which is inconsistent with what our LICENSE says:

At the same time, all these new CI-only files are explicitly excluded from archives generated by git-archive, so tarballs remain consistent.

The rest of the files in this package fall under the following terms
(heavily cut-down "BSD license"):

Redistribution and use in source and binary forms, with or without
modification, are permitted.

Perhaps this inconsistency is now for you to address in one way or another?

I suggest the following wording:

Files in ci directory (install-dependencies.sh and run-build-and-tests.sh) are provided under the terms of the GNU General Public License version 2 or later. These files are not included into archives generated by git-archive.

  1. The clang warning you mention was also seen with some versions of gcc (some 4.x and 5.x) and with MSVC, possibly more. I wonder if this same fix will work for those - somehow I doubt it, and we might need to use an #if instead (if we care to fix this). I'll test with those gcc soon (don't intend to do another Windows build soon, so won't test there soon).

The only reason I made the fix is to enable -Werror in CI.

solardiz commented 3 years ago

tarballs remain consistent.

Actually, this brings up the question on how to create tarballs for passwdqc releases. So far, I made them in the same way as before the project migrated to git - as I got the files into Owl CVS for testing anyway, I just took archives/passwdqc-2.0.1.tar.gz that's created as part of running Owl's make PACKAGE=passwdqc. As a result, it contains whatever files I took from git and into CVS for this, and so far this was all files with no exclusions.

I guess you suggest we use git-archive now? What specific command would you use?

Also, I guess you probably don't care which files get into Owl and which don't?

FWIW, for JtR jumbo I opted to include the CI-related files into its source code tarballs. This makes sense to me because they are in fact part of the source code we wrote and maintain, even if they're unneeded for users building from such source releases. Wouldn't it make sense to do the same for passwdqc?

I am fine with your suggested wording for LICENSE. If we decide to include the CI files into archives, then we should drop the last sentence accordingly.

ldv-alt commented 3 years ago

On Fri, Mar 12, 2021 at 07:04:21AM -0800, Solar Designer wrote:

tarballs remain consistent.

Actually, this brings up the question on how to create tarballs for passwdqc releases. So far, I made them in the same way as before the project migrated to git - as I got the files into Owl CVS for testing anyway, I just took archives/passwdqc-2.0.1.tar.gz that's created as part of running Owl's make PACKAGE=passwdqc. As a result, it contains whatever files I took from git and into CVS for this, and so far this as all files with no exclusions.

I guess you suggest we use git-archive now? What specific command would you use?

Just something as simple as git archive --prefix=passwdqc-2.0.2/ PASSWDQC_2_0_2 would do.

Personally, I do not care about tarballs that much, but there is a tradition to release tarballs, and some projects (e.g. Gentoo, IIRC) still rely on tarballs in their workflow.

Those who build directly from git would appreciate signed tag objects more than release tarballs.

Also, I guess you probably don't care which files get into Owl and which don't?

As I don't build Owl from source nowadays, it's probably not that important indeed.

FWIW, for JtR jumbo I opted to include the CI-related files into its source code tarballs. This makes sense to me because they are in fact part of the source code we wrote and maintain, even if they're unneeded for users building from such source releases. Wouldn't it make sense to do the same for passwdqc?

In strace and linux-pam where we maintain similar CI-related files, these files are not included into release tarballs because we don't see how they could be of any use to tarball consumers.

I am fine with your suggested wording for LICENSE. If we decide to include the CI files into archives, then we should drop the last sentence accordingly.

OK, would you like a separate PR for that?

solardiz commented 3 years ago

git archive --prefix=passwdqc-2.0.2/ PASSWDQC_2_0_2

OK. BTW, isn't it weird that right now this includes po/.gitignore yet not the top-level .gitignore? I guess your intent is to exclude both, although again for JtR jumbo we do it the other way around now.

use to tarball consumers.

Maybe I'm old-fashioned and/or wrong, but I expect tarballs to be as complete snapshots of the project as possible. Sure for the full original source including history the git repo is needed anyway, but I think "snapshot completeness" has value too. A full set of release tarballs provides some history, too, and I don't see why we'd actively exclude a few tiny things from there.

But I don't mind.

Regarding the LICENSE update:

OK, would you like a separate PR for that?

Yes, please.

Ideally, though, maybe you could put the CI stuff (its copy here) under the 0-clause BSD license, so that we don't have to add any special mention to LICENSE?

ldv-alt commented 3 years ago

On Fri, Mar 12, 2021 at 10:54:54AM -0800, Solar Designer wrote:

git archive --prefix=passwdqc-2.0.2/ PASSWDQC_2_0_2

OK. BTW, isn't it weird that right now this includes po/.gitignore yet not the top-level .gitignore?

Yes, but it was corrected by .gitattributes introduced along with "Add github CI" commit. Starting from that commit, all .git* files are excluded from archives produced by git-archive, including those generated by github, see e.g. https://github.com/openwall/passwdqc/archive/main.zip

solardiz commented 3 years ago

The clang warning you mention was also seen with some versions of gcc (some 4.x and 5.x) and with MSVC, possibly more. I wonder if this same fix will work for those - somehow I doubt it

FWIW, the warning is still present with RHEL6'ish "gcc version 4.4.7 20120313 (Red Hat 4.4.7-23) (GCC)" when building for x86_64:

pwqfilter.c: In function 'read_filter':
pwqfilter.c:504: warning: comparison is always false due to limited range of data type