setup and colons in filenames

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

setup and colons in filenames

Ken Brown-6
This is a followup to the thread started here:

   https://cygwin.com/ml/cygwin-patches/2017-q4/msg00012.html

Currently setup's parse_filename is not correctly parsing filenames in
/etc/setup/installed.db that contain colons, as explained in the above
thread.  It would be easy to fix this by just ripping out the 'base'
function, except for the fact that parse_filename is called by
ScanFindVisitor::visitFile.

I don't know enough about WIN32_FIND_DATA to know whether the call to
'base' is needed for that use of parse_filename.  If so, is it safe to
skip all colons in that setting, since we're dealing with Win32
filenames and they don't see the colons in Cygwin filenames?

Do we need two versions of parse_filename, one that calls base and one
that doesn't?

Ken
Reply | Threaded
Open this post in threaded view
|

Re: setup and colons in filenames

Jon TURNEY
On 25/10/2017 16:50, Ken Brown wrote:
> This is a followup to the thread started here:
>
>    https://cygwin.com/ml/cygwin-patches/2017-q4/msg00012.html
>
> Currently setup's parse_filename is not correctly parsing filenames in
> /etc/setup/installed.db that contain colons, as explained in the above
> thread.  It would be easy to fix this by just ripping out the 'base'
> function, except for the fact that parse_filename is called by
> ScanFindVisitor::visitFile.

Since older setup cannot correctly parse an installed.db containing
filenames like that, we should probably bump the installed.db version at
the same time as fixing this.

> I don't know enough about WIN32_FIND_DATA to know whether the call to
> 'base' is needed for that use of parse_filename.  If so, is it safe to
> skip all colons in that setting, since we're dealing with Win32
> filenames and they don't see the colons in Cygwin filenames?

Yeah, that's about as far as I got before giving up...

> Do we need two versions of parse_filename, one that calls base and one
> that doesn't?

This might be the easiest solution :)

The other concern I had was if the filenames for the package archives
stored in the download cache end up containing a ':', which I thought
wasn't allowed in windows filenames?
Reply | Threaded
Open this post in threaded view
|

Re: setup and colons in filenames

Corinna Vinschen-2
On Oct 25 20:23, Jon Turney wrote:

> On 25/10/2017 16:50, Ken Brown wrote:
> > This is a followup to the thread started here:
> >
> >    https://cygwin.com/ml/cygwin-patches/2017-q4/msg00012.html
> >
> > Currently setup's parse_filename is not correctly parsing filenames in
> > /etc/setup/installed.db that contain colons, as explained in the above
> > thread.  It would be easy to fix this by just ripping out the 'base'
> > function, except for the fact that parse_filename is called by
> > ScanFindVisitor::visitFile.
>
> Since older setup cannot correctly parse an installed.db containing
> filenames like that, we should probably bump the installed.db version at the
> same time as fixing this.
>
> > I don't know enough about WIN32_FIND_DATA to know whether the call to
> > 'base' is needed for that use of parse_filename.  If so, is it safe to
> > skip all colons in that setting, since we're dealing with Win32
> > filenames and they don't see the colons in Cygwin filenames?
>
> Yeah, that's about as far as I got before giving up...
>
> > Do we need two versions of parse_filename, one that calls base and one
> > that doesn't?
>
> This might be the easiest solution :)
>
> The other concern I had was if the filenames for the package archives stored
> in the download cache end up containing a ':', which I thought wasn't
> allowed in windows filenames?
Colons in Cygwin filenames will have 0xf03a value in WIN32.  The code to
transpose special chars into the private use area at 0xf0XY is in setup,
but I'm not sure if setup is really working correctly with archives
containing a colon.  That is:

- downloading the archive to the local drive with the filename
  transposed (rather than failing to store the file)
- storing the filename with a real colon in installed.db and still
  being able to transpose the filename when fetched from installed.db.

Especially the latter part is interesting since the reverse operation
(transposing back from 0xf0XY to 0xXY) may not be implemented in setup,
yet.  If so, I don't remember.  Somebody will have to test this.


Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: setup and colons in filenames

Jon TURNEY
On 25/10/2017 21:16, Corinna Vinschen wrote:

> On Oct 25 20:23, Jon Turney wrote:
>> On 25/10/2017 16:50, Ken Brown wrote:
>>> This is a followup to the thread started here:
>>>
>>>     https://cygwin.com/ml/cygwin-patches/2017-q4/msg00012.html
>>>
>>> Currently setup's parse_filename is not correctly parsing filenames in
>>> /etc/setup/installed.db that contain colons, as explained in the above
>>> thread.  It would be easy to fix this by just ripping out the 'base'
>>> function, except for the fact that parse_filename is called by
>>> ScanFindVisitor::visitFile.
>>
>> Since older setup cannot correctly parse an installed.db containing
>> filenames like that, we should probably bump the installed.db version at the
>> same time as fixing this.
>>
>>> I don't know enough about WIN32_FIND_DATA to know whether the call to
>>> 'base' is needed for that use of parse_filename.  If so, is it safe to
>>> skip all colons in that setting, since we're dealing with Win32
>>> filenames and they don't see the colons in Cygwin filenames?
>>
>> Yeah, that's about as far as I got before giving up...
>>
>>> Do we need two versions of parse_filename, one that calls base and one
>>> that doesn't?
>>
>> This might be the easiest solution :)
>>
>> The other concern I had was if the filenames for the package archives stored
>> in the download cache end up containing a ':', which I thought wasn't
>> allowed in windows filenames?
>
> Colons in Cygwin filenames will have 0xf03a value in WIN32.  The code to
> transpose special chars into the private use area at 0xf0XY is in setup,
> but I'm not sure if setup is really working correctly with archives
> containing a colon.
I think the package archives are stored using native Windows filenames,
not cygwin filenames (i.e. file:// paths rather than cygfile:// paths),
because the "Local Package Directory" is not necessarily under the
cygwin root, so I'm not sure that transformation applies.

Reply | Threaded
Open this post in threaded view
|

Re: setup and colons in filenames

Corinna Vinschen-2
On Oct 25 21:36, Jon Turney wrote:

> On 25/10/2017 21:16, Corinna Vinschen wrote:
> > On Oct 25 20:23, Jon Turney wrote:
> > > On 25/10/2017 16:50, Ken Brown wrote:
> > > > This is a followup to the thread started here:
> > > >
> > > >     https://cygwin.com/ml/cygwin-patches/2017-q4/msg00012.html
> > > >
> > > > Currently setup's parse_filename is not correctly parsing filenames in
> > > > /etc/setup/installed.db that contain colons, as explained in the above
> > > > thread.  It would be easy to fix this by just ripping out the 'base'
> > > > function, except for the fact that parse_filename is called by
> > > > ScanFindVisitor::visitFile.
> > >
> > > Since older setup cannot correctly parse an installed.db containing
> > > filenames like that, we should probably bump the installed.db version at the
> > > same time as fixing this.
> > >
> > > > I don't know enough about WIN32_FIND_DATA to know whether the call to
> > > > 'base' is needed for that use of parse_filename.  If so, is it safe to
> > > > skip all colons in that setting, since we're dealing with Win32
> > > > filenames and they don't see the colons in Cygwin filenames?
> > >
> > > Yeah, that's about as far as I got before giving up...
> > >
> > > > Do we need two versions of parse_filename, one that calls base and one
> > > > that doesn't?
> > >
> > > This might be the easiest solution :)
> > >
> > > The other concern I had was if the filenames for the package archives stored
> > > in the download cache end up containing a ':', which I thought wasn't
> > > allowed in windows filenames?
> >
> > Colons in Cygwin filenames will have 0xf03a value in WIN32.  The code to
> > transpose special chars into the private use area at 0xf0XY is in setup,
> > but I'm not sure if setup is really working correctly with archives
> > containing a colon.
> I think the package archives are stored using native Windows filenames, not
> cygwin filenames (i.e. file:// paths rather than cygfile:// paths), because
> the "Local Package Directory" is not necessarily under the cygwin root, so
> I'm not sure that transformation applies.
The transposition is handled in mklongpath, which is called in nt_fopen,
or called manually in other callers of nt_wfopen to construct the long
path.  In theory, this should cover all bases...


Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: setup and colons in filenames

Jon TURNEY
In reply to this post by Jon TURNEY
On 25/10/2017 20:23, Jon Turney wrote:

> On 25/10/2017 16:50, Ken Brown wrote:
>> This is a followup to the thread started here:
>>
>>    https://cygwin.com/ml/cygwin-patches/2017-q4/msg00012.html
>>
>> Currently setup's parse_filename is not correctly parsing filenames in
>> /etc/setup/installed.db that contain colons, as explained in the above
>> thread.  It would be easy to fix this by just ripping out the 'base'
>> function, except for the fact that parse_filename is called by
>> ScanFindVisitor::visitFile.
>
> Since older setup cannot correctly parse an installed.db containing
> filenames like that, we should probably bump the installed.db version at
> the same time as fixing this.
>
>> I don't know enough about WIN32_FIND_DATA to know whether the call to
>> 'base' is needed for that use of parse_filename.  If so, is it safe to
>> skip all colons in that setting, since we're dealing with Win32
>> filenames and they don't see the colons in Cygwin filenames?
>
> Yeah, that's about as far as I got before giving up...
>
>> Do we need two versions of parse_filename, one that calls base and one
>> that doesn't?
>
> This might be the easiest solution :)

It gets better:  We only go down the whole route of using
ScanFindVisitor::visitFile() from do_from_local_dir() if we couldn't
find an .ini file.

If there's no .ini file found, we look at every package archive and
parse the version out of the archive filename.

This seems a really odd thing to do, as we've no idea about the
dependencies of these packages, so installing them is unlikely work well.

Removing all the setup.ini files from my package directory, the good
news is that WIN32_FIND_DATA just returns the filename, not a full
pathname, so this use of base() isn't needed, either.

The bad news is that setup then exits (this works ok in 2.882, so
something's got broken somewhere...)

I'm kind of tempted to remove this mis-feature.

Reply | Threaded
Open this post in threaded view
|

Re: setup and colons in filenames

Jon TURNEY
In reply to this post by Corinna Vinschen-2
On 25/10/2017 22:00, Corinna Vinschen wrote:

> On Oct 25 21:36, Jon Turney wrote:
>> On 25/10/2017 21:16, Corinna Vinschen wrote:
>>> On Oct 25 20:23, Jon Turney wrote:
>>>> On 25/10/2017 16:50, Ken Brown wrote:
>>>>> This is a followup to the thread started here:
>>>>
>>>> The other concern I had was if the filenames for the package archives stored
>>>> in the download cache end up containing a ':', which I thought wasn't
>>>> allowed in windows filenames?
>>>
>>> Colons in Cygwin filenames will have 0xf03a value in WIN32.  The code to
>>> transpose special chars into the private use area at 0xf0XY is in setup,
>>> but I'm not sure if setup is really working correctly with archives
>>> containing a colon.
>> I think the package archives are stored using native Windows filenames, not
>> cygwin filenames (i.e. file:// paths rather than cygfile:// paths), because
>> the "Local Package Directory" is not necessarily under the cygwin root, so
>> I'm not sure that transformation applies.
>
> The transposition is handled in mklongpath, which is called in nt_fopen,
> or called manually in other callers of nt_wfopen to construct the long
> path.  In theory, this should cover all bases...

Yes, you are quite correct, and this transformation is happening, so
there is no problem here.

(I think what I did to confuse myself was look at the "Local Package
Directory" in Cygwin shell without even thinking about it, and saw the
filenames being shown with a ':', while thinking they were Windows
filenames...)
Reply | Threaded
Open this post in threaded view
|

Re: setup and colons in filenames

Ken Brown-6
In reply to this post by Jon TURNEY
On 10/26/2017 12:14 PM, Jon Turney wrote:

> On 25/10/2017 20:23, Jon Turney wrote:
>> On 25/10/2017 16:50, Ken Brown wrote:
>>> This is a followup to the thread started here:
>>>
>>>    https://cygwin.com/ml/cygwin-patches/2017-q4/msg00012.html
>>>
>>> Currently setup's parse_filename is not correctly parsing filenames
>>> in /etc/setup/installed.db that contain colons, as explained in the
>>> above thread.  It would be easy to fix this by just ripping out the
>>> 'base' function, except for the fact that parse_filename is called by
>>> ScanFindVisitor::visitFile.
>>
>> Since older setup cannot correctly parse an installed.db containing
>> filenames like that, we should probably bump the installed.db version
>> at the same time as fixing this.
>>
>>> I don't know enough about WIN32_FIND_DATA to know whether the call to
>>> 'base' is needed for that use of parse_filename.  If so, is it safe
>>> to skip all colons in that setting, since we're dealing with Win32
>>> filenames and they don't see the colons in Cygwin filenames?
>>
>> Yeah, that's about as far as I got before giving up...
>>
>>> Do we need two versions of parse_filename, one that calls base and
>>> one that doesn't?
>>
>> This might be the easiest solution :)
>
> It gets better:  We only go down the whole route of using
> ScanFindVisitor::visitFile() from do_from_local_dir() if we couldn't
> find an .ini file.
>
> If there's no .ini file found, we look at every package archive and
> parse the version out of the archive filename.
>
> This seems a really odd thing to do, as we've no idea about the
> dependencies of these packages, so installing them is unlikely work well.
>
> Removing all the setup.ini files from my package directory, the good
> news is that WIN32_FIND_DATA just returns the filename, not a full
> pathname, so this use of base() isn't needed, either.
>
> The bad news is that setup then exits (this works ok in 2.882, so
> something's got broken somewhere...)

What happens is that ScanFindVisitor::visitFile doesn't set various
attributes of the packages it creates (such as SDesc).  This leads to a
crash when the SolvableVersion methods (such as SDesc()) return null
pointers that get dereferenced by PickView.

> I'm kind of tempted to remove this mis-feature.

I agree strongly.  I was about to write an email suggesting the same
thing when I saw your mail.  Unless you're already working on it, I'll
be glad to prepare a patch series that does this and also takes care of
whatever is needed for supporting colons in installed.db

Ken

Reply | Threaded
Open this post in threaded view
|

Re: setup and colons in filenames

Jon TURNEY
On 26/10/2017 18:04, Ken Brown wrote:
> On 10/26/2017 12:14 PM, Jon Turney wrote:
>
>> I'm kind of tempted to remove this mis-feature.
>
> I agree strongly.  I was about to write an email suggesting the same
> thing when I saw your mail.  Unless you're already working on it, I'll
> be glad to prepare a patch series that does this and also takes care of
> whatever is needed for supporting colons in installed.db

Please do so.

Reply | Threaded
Open this post in threaded view
|

Re: setup and colons in filenames

ASSI
In reply to this post by Jon TURNEY
Jon Turney writes:
> This seems a really odd thing to do, as we've no idea about the
> dependencies of these packages, so installing them is unlikely work
> well.

This particular code path was vetoed from getting thrown out last time I
worked in that area since it would break long-standing expectations
w.r.t. local package archoves that have no setup.ini or even *.hint.

> I'm kind of tempted to remove this mis-feature.

In a properly maintained mirror or local archive it is a mis-feature.  I
was told not everyone wants to have to do the extra steps of doing that
maintenance.  At least if we detect what should be a proper package repo
we don't additionally look at all the files anymore.


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

Factory and User Sound Singles for Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds
Reply | Threaded
Open this post in threaded view
|

Re: setup and colons in filenames

Jon TURNEY
On 26/10/2017 18:42, Achim Gratz wrote:
> Jon Turney writes:
>> This seems a really odd thing to do, as we've no idea about the
>> dependencies of these packages, so installing them is unlikely work
>> well.
>
> This particular code path was vetoed from getting thrown out last time I
> worked in that area since it would break long-standing expectations
> w.r.t. local package archoves that have no setup.ini or even *.hint.

Can we have a link to that discussion, please?

>> I'm kind of tempted to remove this mis-feature.
>
> In a properly maintained mirror or local archive it is a mis-feature.  I
> was told not everyone wants to have to do the extra steps of doing that
> maintenance.  At least if we detect what should be a proper package repo
> we don't additionally look at all the files anymore.
Reply | Threaded
Open this post in threaded view
|

Re: setup and colons in filenames

ASSI
Jon Turney writes:
> Can we have a link to that discussion, please?

It's somewhere in that thread from June 2015 related to the changes
between 2.871 and 2.873.  Also one of the intermediate versions got
complaints due to that code path not working correctly.


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
Reply | Threaded
Open this post in threaded view
|

Re: setup and colons in filenames

Corinna Vinschen-2
On Oct 26 20:25, Achim Gratz wrote:
> Jon Turney writes:
> > Can we have a link to that discussion, please?
>
> It's somewhere in that thread from June 2015 related to the changes
> between 2.871 and 2.873.  Also one of the intermediate versions got
> complaints due to that code path not working correctly.

I'm totally willing to retry.  A real mirror, even a local one, won't
have any problems because it has a setup.ini.

It would be pretty neat if genini could be simplified(*), so a mortal user
can just run it with a directory as parameter and it creates a setup.ini
file at the top-level of that dir without too much complaining about
missing sources etc.

(*) Or better: A user version of calm, packaged in the distro.


Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: setup and colons in filenames

ASSI
Corinna Vinschen writes:
> It would be pretty neat if genini could be simplified(*), so a mortal user
> can just run it with a directory as parameter and it creates a setup.ini
> file at the top-level of that dir without too much complaining about
> missing sources etc.
>
> (*) Or better: A user version of calm, packaged in the distro.

For better or worse, genini is dead and replaced by mksetupini.  The
latter is way more picky with having the correct hint files around and
often refuses to create a setup.ini if it finds something it doesn't
like.


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

Wavetables for the Terratec KOMPLEXER:
http://Synth.Stromeko.net/Downloads.html#KomplexerWaves
Reply | Threaded
Open this post in threaded view
|

Re: setup and colons in filenames

Jon TURNEY
In reply to this post by ASSI
On 26/10/2017 19:25, Achim Gratz wrote:

> Jon Turney writes:
>> Achim Gratz wrote:
>>> Jon Turney writes:
>>>> This seems a really odd thing to do, as we've no idea about the
>>>> dependencies of these packages, so installing them is unlikely work
>>>> well.
>>>
>>> This particular code path was vetoed from getting thrown out last time I
>>> worked in that area since it would break long-standing expectations
>>> w.r.t. local package archives that have no setup.ini or even *.hint.
>>
>>> Can we have a link to that discussion, please?
>>
>> It's somewhere in that thread from June 2015 related to the changes
>> between 2.871 and 2.873.  Also one of the intermediate versions got
>> complaints due to that code path not working correctly.

Using your clues, the only seemingly relevant discussion I found was
starting at [1], but I can't find where it says anything like that.

https://cygwin.com/ml/cygwin-apps/2015-06/msg00091.html
Reply | Threaded
Open this post in threaded view
|

Re: setup and colons in filenames

Jon TURNEY
In reply to this post by ASSI
On 27/10/2017 11:40, Achim Gratz wrote:
> Corinna Vinschen writes:
>> It would be pretty neat if genini could be simplified(*), so a mortal user
>> can just run it with a directory as parameter and it creates a setup.ini
>> file at the top-level of that dir without too much complaining about
>> missing sources etc.

I've worked on [1] a bit so it now contains some instructions how to do
this, rather than the previous state of affairs, which was "genini
exists, work it out yourself"

[1] https://cygwin.com/package-server.html#overlay

>> (*) Or better: A user version of calm, packaged in the distro.

Um, we have had this for a while [2], mksetupini is the tool to use if
you don't need all the extra stuff which calm does.

[2] https://cygwin.com/ml/cygwin-apps/2016-07/msg00047.html

I guess mksetupini could have more friendly defaults to make the command
lines in [1] shorter if that really scares people off (e.g. default
--inifile to stdout, --releasearea to '.' and to
--okmissing=required-package (then need a way to turn it back on, though))

> For better or worse, genini is dead and replaced by mksetupini.  The
> latter is way more picky with having the correct hint files around and
> often refuses to create a setup.ini if it finds something it doesn't
> like.

Not dead, just resting.  Until someone patches it to understand pvr.hint
files.

I make no apologies for mksetupini being more strict, though.
Reply | Threaded
Open this post in threaded view
|

Re: setup and colons in filenames

Ken Brown-6
On 10/27/2017 9:44 AM, Jon Turney wrote:

> I've worked on [1] a bit so it now contains some instructions how to do
> this, rather than the previous state of affairs, which was "genini
> exists, work it out yourself"
>
> [1] https://cygwin.com/package-server.html#overlay
>
>>> (*) Or better: A user version of calm, packaged in the distro.
>
> Um, we have had this for a while [2], mksetupini is the tool to use if
> you don't need all the extra stuff which calm does.
>
> [2] https://cygwin.com/ml/cygwin-apps/2016-07/msg00047.html
>
> I guess mksetupini could have more friendly defaults to make the command
> lines in [1] shorter if that really scares people off (e.g. default
> --inifile to stdout, --releasearea to '.' and to
> --okmissing=required-package (then need a way to turn it back on, though))

I don't see why this should be necessary.  You've written out the
command line for people to copy and paste.

With [1] available, I think it should be OK to rip out ScanFindVisitor.

Ken
Reply | Threaded
Open this post in threaded view
|

Re: setup and colons in filenames

Corinna Vinschen-2
On Oct 27 11:20, Ken Brown wrote:

> On 10/27/2017 9:44 AM, Jon Turney wrote:
> > I've worked on [1] a bit so it now contains some instructions how to do
> > this, rather than the previous state of affairs, which was "genini
> > exists, work it out yourself"
> >
> > [1] https://cygwin.com/package-server.html#overlay
> >
> > > > (*) Or better: A user version of calm, packaged in the distro.
> >
> > Um, we have had this for a while [2], mksetupini is the tool to use if
> > you don't need all the extra stuff which calm does.
> >
> > [2] https://cygwin.com/ml/cygwin-apps/2016-07/msg00047.html
> >
> > I guess mksetupini could have more friendly defaults to make the command
> > lines in [1] shorter if that really scares people off (e.g. default
> > --inifile to stdout, --releasearea to '.' and to
> > --okmissing=required-package (then need a way to turn it back on,
> > though))
>
> I don't see why this should be necessary.  You've written out the command
> line for people to copy and paste.
>
> With [1] available, I think it should be OK to rip out ScanFindVisitor.
>
> Ken
ACK


Corinna

--
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

signature.asc (836 bytes) Download Attachment