Обсуждение: 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@gmx.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@gmx.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@gmx.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.paquier@gmail.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@gmail.com> wrote:
> On Feb 22, 2015, at 5:41 AM, Michael Paquier <michael.paquier@gmail.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.GT1768@alvh.no-ip.org#20141212134508.GT1768@alvh.no-ip.org
--
http://www.postgresql.org/message-id/flat/20141212134508.GT1768@alvh.no-ip.org#20141212134508.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@gmail.com > <mailto:robertmhaas@gmail.com>> wrote: > > > > On Feb 22, 2015, at 5:41 AM, Michael Paquier <michael.paquier@gmail.com <mailto:michael.paquier@gmail.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.GT1768@alvh.no-ip.org#20141212134508.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@gmx.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.paquier@gmail.com) wrote: > On Wed, Feb 25, 2015 at 4:27 AM, Peter Eisentraut <peter_e@gmx.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@snowman.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.paquier@gmail.com) wrote: > On Sun, Mar 1, 2015 at 2:38 AM, Stephen Frost <sfrost@snowman.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@snowman.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