Re: cygport patches for consideration

[Yaakov Selkowitz <yselkowitz@cygwin.com>]

[-- Attachment #1: Type: text/plain, Size: 5544 bytes --]

On Sun, 2020-04-05 at 20:54 +0200, Achim Gratz wrote:
> I've prepared a branch on top of current master for your perusal:
> 
> https://repo.or.cz/cygport/rpm-style.git/shortlog/refs/heads/to-upstream
> 
> As you can see they've been battle-tested by me for quite some time already:
> 
> commit 779a7dd2fc834d45fb0f46cded647557ece17d8f (to-upstream)
> Date:   Sat Apr 20 20:02:33 2013 +0200
> 
>     switch submodule gnuconfig to HTTPS protocol
>     
>     Live more easily with stupid firewalls...

Pushed to master.

> commit fd17952c457b808e360e5ed75ffae8d9571283ad
> Date:   Sun Dec 6 13:32:38 2015 +0100
> 
>     lib/pkg_pkg.cygpart: uniquify requirements after the version has been stripped off
>     
>     * lib/pkg_pkg.cygpart: Move the "sort -fu" command to after the
>       stripping of the version part.  Otherwise some dependencies might
>       get listed twice (perl_base does this sometimes).

Please no CVS style commit messages, I haven't done that in cygport for
over a decade.  Code itself looks okay though.

> commit 40296640fdd0951d72ae0c107f2a46bbf8111ca3
> Date:   Wed Oct 3 08:56:42 2012 +0200
> 
>     support subdirectories in CPAN download URL
>     
>     * cygclass/perl.cygclass: Allow CPAN_AUTHOR to have an /... suffix.
>       This is necessary for some modules that put the distribution files
>       in some subdirectory.  Only upcase the actual author name (up until
>       the first "/") and preserve case for the suffix part.  By request of
>       Reini Urban, also add a variable CPAN_DIR that alternately, if set,
>       will be used to concatenate as ${CPAN_AUTHOR}/${CPAN_DIR} before
>       determining the CPAN download URL.  Neither variable should have a
>       slash ("/") at the beginning or the end.

There is no need for two variables to do the same thing.  I like
Reini's idea but let's call it CPAN_SUBDIR instead.  What about the
attached patch 0001?

> commit 22b3f22b92c472df80be0fdcfd3854bfd19e16f8
> Date:   Sun May 18 17:52:10 2014 +0200
> 
>     pkg_info.cygport: correct search order for Perl dependencies
>     
>     * lib/pkg_info.cygpart: Correct search order for Perl dependencies and
>       suppress auto-generation of Perl dependencies when NO_PERL_DEPS is
>       defined.
>     
>     Dependency generation for Perl at least is too simplistic and doesn't
>     take into account that some modules required or used might actually be
>     optional.  It tends to generate too long dependency lists that vary
>     with the Perl distributions already installed.
>     
>     For starters, the search order should be the reverse of
>     @INC to skip dependencies that are built-in to perl already, but that
>     doesn't pick up those modules that are needed with a higher version
>     since only the presence of the module is detected.  Files in site_perl
>     shoud never be searched since these are local installs.  Files in
>     vendor_perl might be useful to check, however due to the version
>     problem it is better to inject the module dpenedencies from the
>     cygport file.  So skip those searches when NO_PERL_DEPS is defined,
>     which it will be for auto-generated cygport files for Perl
>     distributions (the information is pulled from CPAN/MetaCPAN).

Let's improve the auto-detection instead.  What about the attached
patch 0002?

> commit 2e61b06df6bd5a742f92c25b50d335945e17a34b
> Date:   Sat Aug 18 12:43:23 2018 +0200
> 
>     bin/cygport.in: provide all-test / almostall-test commands on CLI for symmetry with package-test / pkg-test

Nak.  almostall is a deprecated alias of all, there is no need for
symmetry here.

> commit 34e6e1f2dcdf45ac4a3f9e70cae1a8290d860cd9
> Date:   Fri Nov 3 21:47:54 2017 +0100
> 
>     Automatically create a test release if the release string starts with a literal "0"
>     
>     * lib/pkg_pkg.cygpart: Test for a literal "0" as the first character
>       in the release string and make a test release if true.

Nak.  There is not necessarily any correlation between a -0.* release
and whether it should be test or not.

> commit 7f6fb93eaa9e4afe9e12bd57ebb3e8a4daa135ff
> Date:   Sat Apr 8 17:00:34 2017 +0200
> 
>     Show pkg_tag in chatter
>     
>     lib/pkg_pkg.cygpart: inform when creating hint files for a test
>     release

Pushed a slightly different approach to master.  Not sure what the
second part of that patch is supposed to be, however, as there is no
$pkg_testrelease variable in cygport.

> commit 01e199380e32f214c2809d3dcc76f51b61de6d01
> Date:   Sat Sep 17 10:07:10 2016 +0200
> 
>     lib/src_install.cygpart: correct test in make_etc_defaults, possibly show diff
>     
>     * lib/src_install.cygpart (make_etc_defaults): The preremove script
>       only removes plain files when they match the default, so the
>       postinstall script must not install files if _anything_ with the
>       same name already exists.  Change the test from '-f' to '-e'.  If
>       /usr/bin/diff is installed and the target is a plain file, show the
>       diff to the default so the user can decide more easily what to do.

The first part is ok once the commit is reworded.  But when would a
user see the output of a postinstall script?  What would make more
sense is to have a utility akin to "rpmconf -a" on RPM-based systems
which allows the user to compare existing files with their
/etc/defaults and choose if and how to merge the differences.

--
Yaakov


[-- Attachment #2: 0001-perl-cpan-subdir.patch --]
[-- Type: text/x-patch, Size: 1260 bytes --]

diff --git a/cygclass/perl.cygclass b/cygclass/perl.cygclass
index 54cd97b..c226432 100644
--- a/cygclass/perl.cygclass
+++ b/cygclass/perl.cygclass
@@ -139,6 +139,11 @@ esac
 #  before inherit()ing perl.cygclass to have any effect.  If set, the package
 #  HOMEPAGE and SRC_URI are set to their usual locations on CPAN.
 #****
+#****v* perl.cygclass/CPAN_SUBDIR
+#  DESCRIPTION
+#  An optional directory component in the CPAN URL.  Some packages have an
+#  additional subdirectory component, which can be specified with this variable.
+#****
 #****v* perl.cygclass/CPAN_VERSION
 #  DESCRIPTION
 #  The published version of the Perl module on CPAN.  It is sometimes
@@ -190,7 +195,7 @@ HOMEPAGE="https://metacpan.org/release/${ORIG_PN}"
 #****
 cpan_author_ftp=${CPAN_AUTHOR^^}
 cpan_author_ver=${CPAN_VERSION:-${VERSION}}
-SRC_URI="mirror://cpan/authors/id/${cpan_author_ftp:0:1}/${cpan_author_ftp:0:2}/${cpan_author_ftp}/${ORIG_PN}-${cpan_author_ver}.${CPAN_TARBALL_SUFFIX:-tar.gz}"
+SRC_URI="mirror://cpan/authors/id/${cpan_author_ftp:0:1}/${cpan_author_ftp:0:2}/${cpan_author_ftp}${CPAN_SUBDIR+/}${CPAN_SUBDIR}/${ORIG_PN}-${cpan_author_ver}.${CPAN_TARBALL_SUFFIX:-tar.gz}"
 SRC_DIR="${ORIG_PN}-${cpan_author_ver}"
 unset cpan_author_ftp cpan_author_ver
 

[-- Attachment #3: 0002-cygport-perl-dep.patch --]
[-- Type: text/x-patch, Size: 1008 bytes --]

diff --git a/lib/pkg_info.cygpart b/lib/pkg_info.cygpart
index 4d6f598..6901571 100644
--- a/lib/pkg_info.cygpart
+++ b/lib/pkg_info.cygpart
@@ -383,14 +383,14 @@ __list_deps() {
 
 	if check_prog perl
 	then
-		pldirs=($(perl -e 'print join(" ",@INC)'))
+		pldirs=($(perl -e 'print join(" ",reverse grep !/site_perl/,@INC)'))
 		pldirs+=" ${DEPS_PATH//:/ }"
 		for pldep in $(find "${@//^_^/ }" -path 'usr/share/doc/*' -prune \
 				${deps_prune} \
 				-o -path 'usr/share/help/*' -prune \
 				-o \( -name '*.pl' -o -name '*.pm' \) -print \
 				-o -type f ! -name '*.*' -executable -exec sed -sne '1{/^#!.*perl.*/F}' '{}' + \
-				| xargs -r sed -ne "s/^[^#]*use \(base\|parent\) ['\"]*\(qw( *\)*\([A-Z][^-)'\";]*\).*/\3/gp;s/^[^#]*\(use\|require\) ['\"]*\([A-Z][^ '\";]*\).*/\2/gp" \
+				| xargs -r sed -ne "s/^use \(base\|parent\) ['\"]*\(qw( *\)*\([A-Z][^-)'\";]*\).*/\3/gp;s/^\(use\|require\) ['\"]*\([A-Z][^ '\";]*\).*/\2/gp" \
 				| sort -u)
 		do
 			for d in ${pldirs[*]/\//${D}/} ${pldirs[*]}