[PATCH cygport 0/3] cygport patches

[PATCH cygport 0/3] cygport patches

[Jon Turney]

Jon Turney (3):
  Pass DEPEND through to .hint for source package as build-depends:
  Don't allow SRC_URI or PATCH_URI to depend on ARCH
  Update documentation of all and almostall

 README              |  4 ++--
 bin/cygport.in      | 26 ++++++++++++++++++++++++++
 lib/help.cygpart    |  3 ++-
 lib/pkg_pkg.cygpart |  5 +++++
 4 files changed, 35 insertions(+), 3 deletions(-)

-- 
2.12.2

[PATCH cygport 1/3] Pass DEPEND through to .hint for source package as build-depends:

[Jon Turney]

Converting a dependency atom to a package name with full generality requires
a database of all the pathnames contained in all packages, so we don't even
try to do that here, leaving that task for downstream processing of the
.hint file...
---
 lib/pkg_pkg.cygpart | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/pkg_pkg.cygpart b/lib/pkg_pkg.cygpart
index 5552040..01dec69 100644
--- a/lib/pkg_pkg.cygpart
+++ b/lib/pkg_pkg.cygpart
@@ -710,6 +710,10 @@ _EOF
 				cat >> ${distdir}/${PN}/${distsubdir}/${pkg_name[${n}]}-${PVR}.hint <<-_EOF
 external-source: ${PN}
 _EOF
+			else
+				cat >> ${distdir}/${PN}/${distsubdir}/${pkg_name[${n}]}-${PVR}.hint <<-_EOF
+build-depends: ${DEPEND}
+_EOF
 			fi
 			if defined ${pkg_message_var}
 			then
@@ -810,6 +814,7 @@ _EOF
 			cat > ${distdir}/${PN}/${PN}-${PVR}.hint <<-_EOF
 category: ${!pkg_category_var:-${CATEGORY}}
 requires:
+build-depends: ${DEPEND}
 sdesc: "${!pkg_summary_var:-${SUMMARY}}"
 ldesc: "${!pkg_description_var:-${DESCRIPTION:-${!pkg_summary_var:-${SUMMARY}}}}"
 skip:
-- 
2.12.2

Re: [PATCH cygport 1/3] Pass DEPEND through to .hint for source package as build-depends:

[Yaakov Selkowitz]

On 2017-05-02 08:13, Jon Turney wrote:
> Converting a dependency atom to a package name with full generality requires
> a database of all the pathnames contained in all packages, so we don't even
> try to do that here, leaving that task for downstream processing of the
> .hint file...

Or should we just replace DEPEND, which allowed atoms which are 
unsupported by setup and whose name is confusing wrt REQUIRES, with a 
BUILDREQUIRES variable which is package-name only?

-- 
Yaakov

Re: [PATCH cygport 1/3] Pass DEPEND through to .hint for source package as build-depends:

[Brian Inglis]

On 2017-05-05 14:42, Yaakov Selkowitz wrote:
> On 2017-05-02 08:13, Jon Turney wrote:
>> Converting a dependency atom to a package name with full generality
>> requires a database of all the pathnames contained in all packages,
>> so we don't even try to do that here, leaving that task for
>> downstream processing of the .hint file...
> Or should we just replace DEPEND, which allowed atoms which are
> unsupported by setup and whose name is confusing wrt REQUIRES, with a
> BUILDREQUIRES variable which is package-name only?

Confusion could be handled by adding DEPENDS contents to DEPEND 
variables after including pkg.cygport.
The current DEPEND atoms are useful for packaged modules, but that 
does not preclude specifying just a pkg name, and maintainers could 
specify either a module or pkg.
Would it be useful to resolve DEPEND to pkg names and add 
"build-requires" or "build-depends" in downstream processing? 

-- 
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada

Re: [PATCH cygport 1/3] Pass DEPEND through to .hint for source package as build-depends:

[Achim Gratz]

Yaakov Selkowitz writes:
> Or should we just replace DEPEND, which allowed atoms which are
> unsupported by setup and whose name is confusing wrt REQUIRES, with a
> BUILDREQUIRES variable which is package-name only?

BUILDREQUIRES sounds OK to me and somewhat more in line with other
packaging systems.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf microQ V2.22R2:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: [PATCH cygport 1/3] Pass DEPEND through to .hint for source package as build-depends:

[Jon Turney]

On 05/05/2017 21:42, Yaakov Selkowitz wrote:
> On 2017-05-02 08:13, Jon Turney wrote:
>> Converting a dependency atom to a package name with full generality
>> requires
>> a database of all the pathnames contained in all packages, so we don't
>> even
>> try to do that here, leaving that task for downstream processing of the
>> .hint file...
>
> Or should we just replace DEPEND, which allowed atoms which are
> unsupported by setup and whose name is confusing wrt REQUIRES, with a
> BUILDREQUIRES variable which is package-name only?

Changing the name to something less confusing seems like a good idea.

I kind of like the current scheme, as it can do some checks when 
cross-compiling as well.

I think that something (calm?) needs to keep a database of all package 
pathnames, to check for pathname collisions between packages (I suspect 
we have some unnoticed cases of that at the moment), so the extra cost 
of doing a conversion from cygport dependency atoms in a .hint to 
package names in setup.ini shouldn't be too much.  It'll be a while 
before that happens, though... :)

People also like to typo this as DEPENDS, so some sort of warning that 
the .cygport sets a variable which has no effect might also be useful.

Re: [PATCH cygport 1/3] Pass DEPEND through to .hint for source package as build-depends:

[Brian Inglis]

On 2017-05-06 05:03, Jon Turney wrote:
> On 05/05/2017 21:42, Yaakov Selkowitz wrote:
>> On 2017-05-02 08:13, Jon Turney wrote:
>>> Converting a dependency atom to a package name with full generality
>>> requires
>>> a database of all the pathnames contained in all packages, so we don't
>>> even
>>> try to do that here, leaving that task for downstream processing of the
>>> .hint file...
>> Or should we just replace DEPEND, which allowed atoms which are
>> unsupported by setup and whose name is confusing wrt REQUIRES, with a
>> BUILDREQUIRES variable which is package-name only?
> Changing the name to something less confusing seems like a good
> idea.
> I kind of like the current scheme, as it can do some checks when
> cross-compiling as well.
> I think that something (calm?) needs to keep a database of all
> package pathnames, to check for pathname collisions between packages
> (I suspect we have some unnoticed cases of that at the moment), so
> the extra cost of doing a conversion from cygport dependency atoms in
> a .hint to package names in setup.ini shouldn't be too much. It'll be
> a while before that happens, though... :)
> People also like to typo this as DEPENDS, so some sort of warning
> that the .cygport sets a variable which has no effect might also be
> useful.

Append to DEPEND with a warning?

-- 
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada

[PATCH cygport 2/3] Don't allow SRC_URI or PATCH_URI to depend on ARCH

[Jon Turney]

The values which SRC_URI and PATCH_URI evaluate to should not change
depending on ARCH, as this will make the source package arch-dependent
---
 bin/cygport.in | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/bin/cygport.in b/bin/cygport.in
index 6cf0122..25d7e94 100644
--- a/bin/cygport.in
+++ b/bin/cygport.in
@@ -409,6 +409,32 @@ then
 	error "${cygportfile} not found.";
 fi
 
+### perform some validation on the .cygport
+
+# SRC_URI and PATCH_URI should not change depending on ARCH, as this will make
+# the source package arch-dependent
+declare -i n
+declare -a VALUE
+ARCHES=("i686" "x86_64" "noarch")
+for VAR in "SRC_URI" "PATCH_URI"
+do
+    n=0
+    while (( n < ${#ARCHES[*]} ))
+    do
+        read -r < <(declare ARCH=${ARCHES[$n]}; declare ARCH_${ARCH}=1; source ${top}/${cygportfile}; eval echo "\$${VAR}")
+        VALUE[$n]=${REPLY}
+        if (( n > 0 ))
+        then
+            if [ "x${VALUE[0]}" != "x${VALUE[$n]}" ]
+            then
+                error "${VAR} appears to depend on ARCH"
+            fi
+        fi
+        n+=1;
+    done
+done
+unset n VALUE ARCHES VAR
+
 ### load .cygport
 source ${top}/${cygportfile} || error "could not read ${cygportfile}"
 ###
-- 
2.12.2

Re: [PATCH cygport 2/3] Don't allow SRC_URI or PATCH_URI to depend on ARCH

[Andrew Schulman]

> The values which SRC_URI and PATCH_URI evaluate to should not change
> depending on ARCH, as this will make the source package arch-dependent

In that case what's the right thing to do when we have arch-specific
patches? For example screen has one, for x86_64 only.

Re: [PATCH cygport 2/3] Don't allow SRC_URI or PATCH_URI to depend on ARCH

[Jon Turney]

On 02/05/2017 14:31, Andrew Schulman wrote:
>> The values which SRC_URI and PATCH_URI evaluate to should not change
>> depending on ARCH, as this will make the source package arch-dependent
>
> In that case what's the right thing to do when we have arch-specific
> patches? For example screen has one, for x86_64 only.

You could wrap C code in the patch in #ifdef __x86_64__ / #endif

However, looking at screen-terminfo-autoconf.patch, it would seem to be 
entirely benign to use that on x86.

I don't quite understand how screen reliably builds on any x86_64 
platform without that patch...

Re: [PATCH cygport 2/3] Don't allow SRC_URI or PATCH_URI to depend on ARCH

[Andrew Schulman]

> On 02/05/2017 14:31, Andrew Schulman wrote:
> >> The values which SRC_URI and PATCH_URI evaluate to should not change
> >> depending on ARCH, as this will make the source package arch-dependent
> >
> > In that case what's the right thing to do when we have arch-specific
> > patches? For example screen has one, for x86_64 only.
> 
> You could wrap C code in the patch in #ifdef __x86_64__ / #endif
> 
> However, looking at screen-terminfo-autoconf.patch, it would seem to be 
> entirely benign to use that on x86.

True. OK, I'll rerelease as 4.5.1-2 with the same source in both arches.

Re: [PATCH cygport 2/3] Don't allow SRC_URI or PATCH_URI to depend on ARCH

[Yaakov Selkowitz]

On 2017-05-02 08:13, Jon Turney wrote:
> The values which SRC_URI and PATCH_URI evaluate to should not change
> depending on ARCH, as this will make the source package arch-dependent

I think this would require a proper src_prep first, so that patches may 
be still be arch-conditionalized there instead.

-- 
Yaakov

Re: [PATCH cygport 2/3] Don't allow SRC_URI or PATCH_URI to depend on ARCH

[Jon Turney]

On 05/05/2017 23:06, Yaakov Selkowitz wrote:
> On 2017-05-02 08:13, Jon Turney wrote:
>> The values which SRC_URI and PATCH_URI evaluate to should not change
>> depending on ARCH, as this will make the source package arch-dependent
>
> I think this would require a proper src_prep first, so that patches may
> be still be arch-conditionalized there instead.

Ok, that makes sense.

So I guess there could be a hook called for each patch to cause it to be 
applied/skipped, or to adjust the patch list?  Or do you think more of 
__src_prep needs to be customizable?

Re: [PATCH cygport 2/3] Don't allow SRC_URI or PATCH_URI to depend on ARCH

[Jon Turney]

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

On 06/05/2017 12:32, Jon Turney wrote:
> On 05/05/2017 23:06, Yaakov Selkowitz wrote:
>> On 2017-05-02 08:13, Jon Turney wrote:
>>> The values which SRC_URI and PATCH_URI evaluate to should not change
>>> depending on ARCH, as this will make the source package arch-dependent
>>
>> I think this would require a proper src_prep first, so that patches may
>> be still be arch-conditionalized there instead.
>
> Ok, that makes sense.
>
> So I guess there could be a hook called for each patch to cause it to be
> applied/skipped, or to adjust the patch list?  Or do you think more of
> __src_prep needs to be customizable?


Attached is an implementation of the first suggestion.



[-- Attachment #2: 0001-Add-src_patch_apply_hook.patch --]
[-- Type: text/plain, Size: 1548 bytes --]

From 4690490e81ddc2c429d3e124639b2814db96f249 Mon Sep 17 00:00:00 2001
From: Jon Turney <jon.turney@dronecode.org.uk>
Date: Sat, 6 May 2017 13:37:13 +0100
Subject: [PATCH cygport] Add src_patch_apply_hook

---
 lib/check_funcs.cygpart |  1 +
 lib/src_prep.cygpart    | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/lib/check_funcs.cygpart b/lib/check_funcs.cygpart
index 93f3e12..bfa02f5 100644
--- a/lib/check_funcs.cygpart
+++ b/lib/check_funcs.cygpart
@@ -687,6 +687,7 @@ __check_unstable() {
 	else
 		warning "The ${1} API is not yet stable and is subject to change"
 		warning "You must define ${unstable_var} in order to use it"
+		return  0
 	fi
 }
 
diff --git a/lib/src_prep.cygpart b/lib/src_prep.cygpart
index d787d2f..aad5564 100644
--- a/lib/src_prep.cygpart
+++ b/lib/src_prep.cygpart
@@ -209,6 +209,24 @@ cygpatch() {
 			continue;
 		fi
 
+#****v* Preparation/src_patch_apply_hook
+#  DESCRIPTION
+#  src_patch_apply_hook() is an optional function which can be defined
+#  to control patch application, for architecture dependent patches, etc.
+#  It receives a patch filename as an argument, and a non-zero result
+#  causes the patch to be skipped.
+#  (This API is not yet stable and is subject to change)
+#****
+
+		if __check_function src_patch_apply_hook
+		then
+			if ! __check_unstable src_patch_apply_hook ${patchfile_name}
+			then
+				warning "patch ${patchfile_name} skipped by src_patch_apply_hook";
+				continue;
+			fi
+		fi
+
 		patchlevel=0;
 
 		case ${patchfile_name} in
-- 
2.12.2

[PATCH cygport 3/3] Update documentation of all and almostall

[Jon Turney]

'all of the above' is no longer accurate since the addition of upload etc.
---
 README           | 4 ++--
 lib/help.cygpart | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/README b/README
index 573276e..fa09363 100644
--- a/README
+++ b/README
@@ -146,8 +146,8 @@ to be processed.  All other arguments are interpreted as a COMMAND, which may be
     package   - create binary and source packages
     upload    - upload finished packages to cygwin.com
     finish    - delete the working directory
-    all       - run all of the above, including finish
-    almostall - run all of the above, except for finish
+    all       - run prep, compile, install, package and finish
+    almostall - as all, excluding finish
 
 Other COMMANDs are meant primarily for maintainers:
 
diff --git a/lib/help.cygpart b/lib/help.cygpart
index 6e167ac..073486c 100644
--- a/lib/help.cygpart
+++ b/lib/help.cygpart
@@ -49,7 +49,8 @@ __show_help() {
 		  upload       upload finished packages to cygwin.com
 		  announce     send an announcement email to cygwin.com
 		  finish       delete the working directory
-		  all          run all of the above, excluding finish
+		  all          run prep, compile, install, package and finish
+		  almostall    as all, excluding finish
 
 		See the included README file for further documentation.
 
-- 
2.12.2

Re: [PATCH cygport 3/3] Update documentation of all and almostall

[Andrew Schulman]

> 'all of the above' is no longer accurate since the addition of upload etc.

In fact, all and almostall are identical. Neither includes finish. See line
627 of /usr/bin/cygport.

Both are misnamed IMO since they exclude get and upload.

Re: [PATCH cygport 3/3] Update documentation of all and almostall

[Jon Turney]

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

On 02/05/2017 15:29, Andrew Schulman wrote:
>> 'all of the above' is no longer accurate since the addition of upload etc.
>
> In fact, all and almostall are identical. Neither includes finish. See line
> 627 of /usr/bin/cygport.
>
> Both are misnamed IMO since they exclude get and upload.

Oh wow, I'd completely missed that change [1].  Just think of all the 
time I could have saved typing 'all' rather than 'almostall' :-)

Revised patch attached.

[1] 
https://github.com/cygwinports/cygport/commit/cf200fb6328323bb3ad5a3d557e11ab7f4203d0c

[-- Attachment #2: 0003-Update-documentation-of-all.patch --]
[-- Type: text/plain, Size: 1515 bytes --]

From 6d2426fe188df265846d93b9881cb208596b6b4b Mon Sep 17 00:00:00 2001
From: Jon Turney <jon.turney@dronecode.org.uk>
Date: Sat, 8 Apr 2017 23:21:19 +0100
Subject: [PATCH cygport 3/3] Update documentation of all

'all of the above' is no longer accurate since the addition of upload etc.

Remove mention almostall which is identical
---
 README           | 3 +--
 lib/help.cygpart | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/README b/README
index 573276e..985e682 100644
--- a/README
+++ b/README
@@ -146,8 +146,7 @@ to be processed.  All other arguments are interpreted as a COMMAND, which may be
     package   - create binary and source packages
     upload    - upload finished packages to cygwin.com
     finish    - delete the working directory
-    all       - run all of the above, including finish
-    almostall - run all of the above, except for finish
+    all       - run prep, compile, install and package
 
 Other COMMANDs are meant primarily for maintainers:
 
diff --git a/lib/help.cygpart b/lib/help.cygpart
index 6e167ac..460f8f7 100644
--- a/lib/help.cygpart
+++ b/lib/help.cygpart
@@ -49,7 +49,7 @@ __show_help() {
 		  upload       upload finished packages to cygwin.com
 		  announce     send an announcement email to cygwin.com
 		  finish       delete the working directory
-		  all          run all of the above, excluding finish
+		  all          run prep, compile, install and package
 
 		See the included README file for further documentation.
 
-- 
2.12.2

Re: [PATCH cygport 3/3] Update documentation of all and almostall

[Yaakov Selkowitz]

On 2017-05-02 10:12, Jon Turney wrote:
> Oh wow, I'd completely missed that change [1].  Just think of all the
> time I could have saved typing 'all' rather than 'almostall' :-)

Sorry. :-S

> Revised patch attached.

Merged.  Thanks,

-- 
Yaakov