Enforce creation of destination folders for source files in pg_regress (Was: pg_regress writes into source tree)

Hi all,

pg_regress will fail with test suites using only source files if the
destination folders do not exist in the code tree. This is annoying
because this forces to maintain empty folders sql/ and expected/ with
a .gitignore ignoring everything. The issue has been discussed here
(http://www.postgresql.org/message-id/54935D9D.7010608@dunslane.net)
but nobody actually sent a patch, so here is one, and a thread for
this discussion.
Regards,
--
Michael

On 1/14/15 11:31 PM, Michael Paquier wrote:
> pg_regress will fail with test suites using only source files if the
> destination folders do not exist in the code tree. This is annoying
> because this forces to maintain empty folders sql/ and expected/ with
> a .gitignore ignoring everything.

We'd still need the .gitignore files somewhere. Do you want to move
them one directory up?

On Fri, Feb 20, 2015 at 5:50 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On 1/14/15 11:31 PM, Michael Paquier wrote:
>> pg_regress will fail with test suites using only source files if the
>> destination folders do not exist in the code tree. This is annoying
>> because this forces to maintain empty folders sql/ and expected/ with
>> a .gitignore ignoring everything.
>
> We'd still need the .gitignore files somewhere. Do you want to move
> them one directory up?

I am not sure I am getting what you are pointing to... For extensions
that already have non-empty sql/ and expected/, they should have their
own ignore entries as sql/.gitignore and expected/.gitignore. The
point of the patch is to simplify the code tree of extensions that
need to keep empty sql/ and expected/, for example to be able to run
regression tests after a fresh repository clone for example.
--
Michael

On 2/20/15 1:56 AM, Michael Paquier wrote:
>> We'd still need the .gitignore files somewhere. Do you want to move
>> them one directory up?
>
> I am not sure I am getting what you are pointing to... For extensions
> that already have non-empty sql/ and expected/, they should have their
> own ignore entries as sql/.gitignore and expected/.gitignore. The
> point of the patch is to simplify the code tree of extensions that
> need to keep empty sql/ and expected/, for example to be able to run
> regression tests after a fresh repository clone for example.

The affected modules have sql/.gitignore and/or expected/.gitignore
files, so the case that the directory doesn't exist and needs to be
created doesn't actually happen.

You could argue that these .gitignore files don't actually belong there,
but your patch doesn't change or move those files, and even modules that
have non-empty sql/ or expected/ directories have .gitignore files
there, so it is considered the appropriate location.

On Sat, Feb 21, 2015 at 6:51 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On 2/20/15 1:56 AM, Michael Paquier wrote:
>>> We'd still need the .gitignore files somewhere. Do you want to move
>>> them one directory up?
>>
>> I am not sure I am getting what you are pointing to... For extensions
>> that already have non-empty sql/ and expected/, they should have their
>> own ignore entries as sql/.gitignore and expected/.gitignore. The
>> point of the patch is to simplify the code tree of extensions that
>> need to keep empty sql/ and expected/, for example to be able to run
>> regression tests after a fresh repository clone for example.
>
> The affected modules have sql/.gitignore and/or expected/.gitignore
> files, so the case that the directory doesn't exist and needs to be
> created doesn't actually happen.

What about extensions that do not have sql/ and extended/ in their
code tree? I don't have an example of such extensions in my mind but I
cannot assume either that they do not exist. See for example something
that happended a couple of months back, dummy_seclabel has been
trapped by that with df761e3, fixed afterwards with 3325624 (the
structure has been changed again since, but that's the error showed up
because of those missing sql/ and expected/).

> You could argue that these .gitignore files don't actually belong there,
> but your patch doesn't change or move those files, and even modules that
> have non-empty sql/ or expected/ directories have .gitignore files
> there, so it is considered the appropriate location.

This is up to the maintainer of each extension to manage their code
tree. However I can imagine that some people would be grateful if we
allow them to not need sql/ and expected/ containing only one single
.gitignore file ignoring everything with "*", making the code tree of
their extensions cleaner.
--
Michael

On 2/22/15 5:41 AM, Michael Paquier wrote:
>> You could argue that these .gitignore files don't actually belong there,
>> but your patch doesn't change or move those files, and even modules that
>> have non-empty sql/ or expected/ directories have .gitignore files
>> there, so it is considered the appropriate location.
>
> This is up to the maintainer of each extension to manage their code
> tree. However I can imagine that some people would be grateful if we
> allow them to not need sql/ and expected/ containing only one single
> .gitignore file ignoring everything with "*", making the code tree of
> their extensions cleaner.

I would like to have an extension in tree that also does this, so we
have a regression test of this functionality.

On Mon, Feb 23, 2015 at 12:00 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On 2/22/15 5:41 AM, Michael Paquier wrote:
>>> You could argue that these .gitignore files don't actually belong there,
>>> but your patch doesn't change or move those files, and even modules that
>>> have non-empty sql/ or expected/ directories have .gitignore files
>>> there, so it is considered the appropriate location.
>>
>> This is up to the maintainer of each extension to manage their code
>> tree. However I can imagine that some people would be grateful if we
>> allow them to not need sql/ and expected/ containing only one single
>> .gitignore file ignoring everything with "*", making the code tree of
>> their extensions cleaner.
>
> I would like to have an extension in tree that also does this, so we
> have a regression test of this functionality.

Sure. Here is one in the patch attached added as a test module. The
name of the module is regress_dynamic. Perhaps the name could be
better..
--
Michael

> On Feb 22, 2015, at 5:41 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
> This is up to the maintainer of each extension to manage their code
> tree. However I can imagine that some people would be grateful if we
> allow them to not need sql/ and expected/ containing only one single
> .gitignore file ignoring everything with "*", making the code tree of
> their extensions cleaner.

I don't see this as an improvement. What's wrong with a .gitignore file ignoring everything?

...Robert

On Mon, Feb 23, 2015 at 9:31 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

>
> > On Feb 22, 2015, at 5:41 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
> wrote:
> > This is up to the maintainer of each extension to manage their code
> > tree. However I can imagine that some people would be grateful if we
> > allow them to not need sql/ and expected/ containing only one single
> > .gitignore file ignoring everything with "*", making the code tree of
> > their extensions cleaner.
>
> I don't see this as an improvement. What's wrong with a .gitignore file
> ignoring everything?
>

That's mainly a matter of code maintenance style (and I am on the side that
a minimum number of folders in a git tree makes things easier to follow),
and IMO it is strange to not have some flexibility. Btw, the reason why
this patch exists is this thread of last December:
http://www.postgresql.org/message-id/flat/20141212134508(dot)GT1768(at)alvh(dot)no-ip(dot)org#20141212134508(dot)GT1768@alvh.no-ip.org
--
Michael

On 2/23/15 1:27 AM, Michael Paquier wrote:
>> I would like to have an extension in tree that also does this, so we
>> > have a regression test of this functionality.
> Sure. Here is one in the patch attached added as a test module. The
> name of the module is regress_dynamic. Perhaps the name could be
> better..

I was more thinking like we could use an existing module like file_fdw.

On 2/24/15 1:23 AM, Michael Paquier wrote:
>
>
> On Mon, Feb 23, 2015 at 9:31 PM, Robert Haas <robertmhaas(at)gmail(dot)com
> <mailto:robertmhaas(at)gmail(dot)com>> wrote:
>
>
> > On Feb 22, 2015, at 5:41 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com <mailto:michael(dot)paquier(at)gmail(dot)com>> wrote:
> > This is up to the maintainer of each extension to manage their code
> > tree. However I can imagine that some people would be grateful if we
> > allow them to not need sql/ and expected/ containing only one single
> > .gitignore file ignoring everything with "*", making the code tree of
> > their extensions cleaner.
>
> I don't see this as an improvement. What's wrong with a .gitignore
> file ignoring everything?
>
>
> That's mainly a matter of code maintenance style (and I am on the side
> that a minimum number of folders in a git tree makes things easier to
> follow), and IMO it is strange to not have some flexibility. Btw, the
> reason why this patch exists is this thread of last December:
> http://www.postgresql.org/message-id/flat/20141212134508(dot)GT1768(at)alvh(dot)no-ip(dot)org#20141212134508(dot)GT1768@alvh.no-ip.org

What's an example of this? I ask because what I'd like is to break the
cluster management portion of pg_regress out on it's own to make it easy
to get that functionality while using a different test framework (like
pgTap).
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On Wed, Feb 25, 2015 at 4:27 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On 2/23/15 1:27 AM, Michael Paquier wrote:
>>> I would like to have an extension in tree that also does this, so we
>>> > have a regression test of this functionality.
>> Sure. Here is one in the patch attached added as a test module. The
>> name of the module is regress_dynamic. Perhaps the name could be
>> better..
>
> I was more thinking like we could use an existing module like file_fdw.

Right. I forgot this one. A patch is attached to do so.
--
Michael

* Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> On Wed, Feb 25, 2015 at 4:27 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> > On 2/23/15 1:27 AM, Michael Paquier wrote:
> >>> I would like to have an extension in tree that also does this, so we
> >>> > have a regression test of this functionality.
> >> Sure. Here is one in the patch attached added as a test module. The
> >> name of the module is regress_dynamic. Perhaps the name could be
> >> better..
> >
> > I was more thinking like we could use an existing module like file_fdw.
>
> Right. I forgot this one. A patch is attached to do so.

I'm always doing VPATH builds, but I believe there's still an
outstanding question on this from Peter about if we need to add entries
to .gitignore for, eg, sql/file_fdw.sql if these directories don't exist
because otherwise those would show up to someone doing a git status and
to Robert's point- if we need those anyway, it's probably better to have
them be in those directories rather than up a level and referring to
directories that don't exist in the tree.

Having the extra directories already exist doesn't seem like a bad thing
to me anyway. If they are causing confusion or something then perhaps a
README could be added to let people know why they exist.

I'm going to mark this back to 'waiting on author' in case there's
something material that I've missed which you can clarify. I had
started this review thinking to help move it along but after re-reading
the thread and thinking about it for a bit, I came around to the other
side and therefore think it should probably be rejected. We certainly
appreciate the effort, of course.

Thanks!

Stephen

On Sun, Mar 1, 2015 at 2:38 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> I'm going to mark this back to 'waiting on author' in case there's
> something material that I've missed which you can clarify. I had
> started this review thinking to help move it along but after re-reading
> the thread and thinking about it for a bit, I came around to the other
> side and therefore think it should probably be rejected. We certainly
> appreciate the effort, of course.

Re-reading this thread there are 3 perplexed opinions from three
different committers, and nobody pleading in favor of this patch
except me, so let's simply mark this as rejected and move on.
--
Michael

* Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> On Sun, Mar 1, 2015 at 2:38 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > I'm going to mark this back to 'waiting on author' in case there's
> > something material that I've missed which you can clarify. I had
> > started this review thinking to help move it along but after re-reading
> > the thread and thinking about it for a bit, I came around to the other
> > side and therefore think it should probably be rejected. We certainly
> > appreciate the effort, of course.
>
> Re-reading this thread there are 3 perplexed opinions from three
> different committers, and nobody pleading in favor of this patch
> except me, so let's simply mark this as rejected and move on.

Works for me.

Thanks!

Stephen

Michael Paquier wrote:
> On Sun, Mar 1, 2015 at 2:38 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > I'm going to mark this back to 'waiting on author' in case there's
> > something material that I've missed which you can clarify. I had
> > started this review thinking to help move it along but after re-reading
> > the thread and thinking about it for a bit, I came around to the other
> > side and therefore think it should probably be rejected. We certainly
> > appreciate the effort, of course.
>
> Re-reading this thread there are 3 perplexed opinions from three
> different committers, and nobody pleading in favor of this patch
> except me, so let's simply mark this as rejected and move on.

Well I also don't really like the existing behavior but it's not a big
issue and so I'm OK with letting go.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services