[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cygwin-apps
Subject:    Re: cygport
From:       Achim Gratz <Stromeko () nexgo ! de>
Date:       2022-09-15 19:34:18
Message-ID: 87o7vg4ekl.fsf () Rainer ! invalid
[Download RAW message or body]

Jon Turney writes:
> I'm not keen on reviewing patches supplied that way, but ok...

Suggest an alternative perhaps?

> > 5bc72c1 lib/pkg_pkg: allow suppression of spurious package dependencies
> 
> As far as I recall, previous discussion of this petered out with
> "please give a concrete example where the dependency autodetection is
> wrong, and explain why it can't be fixed".
> 
> If I'm misremembering, please point that out.

I need to use this in perl.cygport or else perl_base creates a circular
dependency to perl.  Perl has several modules (the official one is
Module::ScanDeps) that try to figure out which deppendencies a Perl
program might have, but even these produce both false positives and
negatives.  The simplistic grep/sed filter that fishes out everything
that looks like a use or require statement is mostly produces  false
positives, since it doesn't consider conditionals.

> The new variable this introduces needs documenting.
> 
> > f5764cf lib/pkg_info.cygport: implement automatic determination of the \
> > appropriate perl5_0xy requirement
> 
> This looks like a fix for the previous commit, mixed in with some
> rebase detritus.

Yeah, I guess the pkg_bin_requires part hopped over the previous patch.

> > f1fdfb4 lib/src_prep.cygpart: process various checksum digests
> 
> The change to cygpatch to allow .xz and .zst compressed patches needs
> breaking out as a separate change.  That should also improve the
> documentation of PATCH_URI to mention that it handles compressed patch
> files transparently.

OK.

> This needs a documentation change to mention when prep will verify
> checksums.
> 
> It seems that __chksum_verify() ignores the result of running *sum.  Why?

The test is not very robust against hand-produced or otherwise slightly
modified checksum files.  In any case it follows the example of the GPG
signature check in that regard.

> Ideally, a test should be added or extended to exercise this.
> 
> > 63e00e5 lib/src_prep.cygpart: determine and deal correctly with another type of \
> > checksum
> 
> This should be combined with the previous patch?

Hysterical raisins… that was in fact added later since I hadn't
encountered one of those before.  I can merge the two patches no
problem.

> > 8a325c5 bin/cygport.in: make system-wide defaults overrideable by user defaults \
> > and provide ability to change initial MAKEOPTS via CYGPORT_MAKEOPTS
> 
> This seems to be two separate changes.
> 
> The documentation for cygport.conf should be updated to reflect that
> ~/.cygport.conf overrides /etc/cygport.conf.
> 
> What it the use case for being able to override MAKEOPTS with a
> environment variable?

I've ran into trouble with Makefiles that are not parallel safe a few
times, so I wanted a way of playing with that without having to modify
the cygport each time.  It's more convenient to specify that on the
command line.

> CYGPORT_MAKEOPTS needs documenting.

OK.

> > 74935d6 bin/cygport.in, lib/help.cygpart: implement --jobs/-j N option to specify \
> > number of jobs to use
> 
> This seems to contain part of the previous change, removing the break
> when looping over config files.
> 
> Why do we need both -j and CYGPORT_MAKEOPTS?

Well strictly we don't need it, ensuring that the approrpiate number of
processors get used is important to me for parallel package builds (the
more packages that can be built in parallel, the less number of cores
each individual job gets assigned).

CYGPORT_MAKEOPTS was introduced and proved useful for debugging much
earlier, so I kept it around even though I don't want to use it for the
purpose of just controlling the number of job slots.

> > 7777191 allow for different package compression types and implement ZStandard \
> > decompression
> 
> The change to unpack .tar.zst or .zst sources needs to be separate.

OK.

> Ideally, a test should be added or extended to exercise that.
> 
> If the intention is to set CYGPORT_TAR_EXT and CYGPORT_TAR_CMD in the
> cygport, I don't think they need the CYGPORT_ prefix.

Since these variables bleed into the environment I'd like to play it safe.

> These variables need documenting.
> 
> This seems like a weird implementation to me.  Why can't cygport set
> TAR_CMD to invoke tar with the appropriate compression option,
> depending on what TAR_EXT is set to?

Because not all tar implementations support auto-compression based on
the extension.  In particular, Cygwin's tar did not yet recognise .zst
as a valid extension when this was introduced.  The other reason is that
with auto-compression the compression level can not be modified for
several compression types and it's useful to change some other things
via this facility.  This was briefly discussed when the patch was posted
initially:

https://inbox.sourceware.org/cygwin-apps/87tuzd7vyp.fsf@Rainer.invalid/

> > 34502d2 (stromenko/to-upstream) lib/src_install.cygpart: make_etc_defaults, \
> > create diff if files aren't matching
> 
> This seems kind of useful, but what's the reasoning behind saving a
> diff vs. just saving a backup copy of the previous file?

You mean like rpm does?  The first thing I always do with .rpmnew or
.rpmsave is to run a diff so I can see which changes to keep and which
to skip.

> The documentation for make_etc_defaults should mention this behaviour.

OK.


This will take a while I think.


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

Samples for the Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#BlofeldSamplesExtra


[prev in list] [next in list] [prev in thread] [next in thread]