Thread: Cygwin PostgreSQL ESQL Patch
The first part of the attached patch solves the Cygwin PostgreSQL ESQL
problem reported by Michael Lemke. See the Cygwin mailing list thread
that begins with the following:
http://sources.redhat.com/ml/cygwin/2001-04/msg00827.html
The second part of the patch tweaks
src/interfaces/ecpg/test/Makefile
so the test ESQL programs build under Cygwin too. The most notable
changes cause the libraries to be listed after the sources file on
the cc/gcc command lines.
Note that I tried the patch under Red Hat 6.2 Linux without any ill
effects.
Thanks,
Jason
--
Jason Tishler
Director, Software Engineering Phone: +1 (732) 264-8770 x235
Dot Hill Systems Corp. Fax: +1 (732) 264-8798
82 Bethany Road, Suite 7 Email: Jason.Tishler@dothill.com
Hazlet, NJ 07730 USA WWW: http://www.dothill.com
Attachment
Jason Tishler <Jason.Tishler@dothill.com> writes:
> --- include/sqlca.h 2001/03/22 06:16:20 1.16
> +++ include/sqlca.h 2001/04/20 14:15:01
> @@ -1,6 +1,12 @@
> #ifndef POSTGRES_SQLCA_H
> #define POSTGRES_SQLCA_H
> +#ifdef __CYGWIN__
> +#define DLLIMPORT __declspec (dllimport)
> +#else
> +#define DLLIMPORT
> +#endif /* __CYGWIN__ */
> +
> #define SQLERRMC_LEN 70
This seems rather bad --- why should sqlca.h define this symbol? What
happens if sqlca.h is included in a file that also includes other
Postgres includes, which will probably pull in c.h where the real
definition is? I'd suggest adding
#include "postgres_ext.h"
#include "c.h"
instead, which may be more namespace pollution than we'd like, but
at least it does not create a potential definition conflict.
regards, tom lane
Tom,
On Fri, Apr 20, 2001 at 12:43:30PM -0400, Tom Lane wrote:
> Jason Tishler <Jason.Tishler@dothill.com> writes:
> > --- include/sqlca.h 2001/03/22 06:16:20 1.16
> > +++ include/sqlca.h 2001/04/20 14:15:01
> > @@ -1,6 +1,12 @@
> > #ifndef POSTGRES_SQLCA_H
> > #define POSTGRES_SQLCA_H
>
> > +#ifdef __CYGWIN__
> > +#define DLLIMPORT __declspec (dllimport)
> > +#else
> > +#define DLLIMPORT
> > +#endif /* __CYGWIN__ */
> > +
> > #define SQLERRMC_LEN 70
>
> This seems rather bad
Sorry, but I *did* try to start a discussion to determine the best way
to resolve this issue. See the following for details:
http://postgresql.readysetnet.com/mhonarc/pgsql-cygwin/2001-04/msg00004.html
Since no one responded, I took my best WAG.
> --- why should sqlca.h define this symbol? What
> happens if sqlca.h is included in a file that also includes other
> Postgres includes, which will probably pull in c.h where the real
> definition is?
I never thought about this possibility -- I erroneously thought that
sqlca.h was used by ESQL only.
> I'd suggest adding
>
> #include "postgres_ext.h"
> #include "c.h"
>
> instead, which may be more namespace pollution than we'd like, but
> at least it does not create a potential definition conflict.
In the above mentioned URL, I suggested including c.h as another
possibly better solution. However, I saw the following in ecpglib.h:
/*
* this is a small part of c.h since we don't want to leak all postgres
* definitions into ecpg programs
*/
so I decided against including c.h in the first version of my patch.
Nevertheless, I appreciate your guidance and I will redo my patch as
you suggested.
Thanks,
Jason
--
Jason Tishler
Director, Software Engineering Phone: +1 (732) 264-8770 x235
Dot Hill Systems Corp. Fax: +1 (732) 264-8798
82 Bethany Road, Suite 7 Email: Jason.Tishler@dothill.com
Hazlet, NJ 07730 USA WWW: http://www.dothill.com
Jason Tishler <Jason.Tishler@dothill.com> writes:
>> --- why should sqlca.h define this symbol? What
>> happens if sqlca.h is included in a file that also includes other
>> Postgres includes, which will probably pull in c.h where the real
>> definition is?
> I never thought about this possibility -- I erroneously thought that
> sqlca.h was used by ESQL only.
AFAICT sqlca.h will be included by some (if not all) ecpg client
programs. I'm not sure whether we should expect that such clients
might also include other Postgres headers. However, sqlca.h is also
included by several modules of ecpg itself, which certainly do include
a ton of Postgres headers. So there is a distinct risk that this will
fail when using a compiler that complains about redefinition of macros.
> In the above mentioned URL, I suggested including c.h as another
> possibly better solution. However, I saw the following in ecpglib.h:
> /*
> * this is a small part of c.h since we don't want to leak all postgres
> * definitions into ecpg programs
> */
> so I decided against including c.h in the first version of my patch.
That's a fair point. Maybe it should be
+#ifndef DLLIMPORT
+#ifdef __CYGWIN__
+#define DLLIMPORT __declspec (dllimport)
+#else
+#define DLLIMPORT
+#endif /* __CYGWIN__ */
+#endif /* DLLIMPORT */
regards, tom lane
On Fri, Apr 20, 2001 at 02:38:53PM -0400, Tom Lane wrote: > Jason Tishler <Jason.Tishler@dothill.com> writes: > > In the above mentioned URL, I suggested including c.h as another > > possibly better solution. However, I saw the following in ecpglib.h: > > /* > > * this is a small part of c.h since we don't want to leak all postgres > > * definitions into ecpg programs > > */ > > so I decided against including c.h in the first version of my patch. > > That's a fair point. Maybe it should be > > +#ifndef DLLIMPORT > +#ifdef __CYGWIN__ > +#define DLLIMPORT __declspec (dllimport) > +#else > +#define DLLIMPORT > +#endif /* __CYGWIN__ */ > +#endif /* DLLIMPORT */ I will redo my patch with the modification suggested above. Thanks, Jason -- Jason Tishler Director, Software Engineering Phone: +1 (732) 264-8770 x235 Dot Hill Systems Corp. Fax: +1 (732) 264-8798 82 Bethany Road, Suite 7 Email: Jason.Tishler@dothill.com Hazlet, NJ 07730 USA WWW: http://www.dothill.com
Tom, On Fri, Apr 20, 2001 at 03:38:56PM -0400, Jason Tishler wrote: > On Fri, Apr 20, 2001 at 02:38:53PM -0400, Tom Lane wrote: > > That's a fair point. Maybe it should be > > > > +#ifndef DLLIMPORT > > +#ifdef __CYGWIN__ > > +#define DLLIMPORT __declspec (dllimport) > > +#else > > +#define DLLIMPORT > > +#endif /* __CYGWIN__ */ > > +#endif /* DLLIMPORT */ > > I will redo my patch with the modification suggested above. I have redone my patch incorporating the above plus I fixed a glaring hole now I know that sqlca.h is included by more then ESQL clients. Without the src/makefiles/Makefile.win part, ecpg.dll no longer built under Cygwin. This time I have rebuilt everything from scratch on both Linux and Cygwin without any errors. I have also rerun the regression tests and all passed. I apologize for the sloppiness of my previous patch -- that will not happen again. Thanks, Jason -- Jason Tishler Director, Software Engineering Phone: +1 (732) 264-8770 x235 Dot Hill Systems Corp. Fax: +1 (732) 264-8798 82 Bethany Road, Suite 7 Email: Jason.Tishler@dothill.com Hazlet, NJ 07730 USA WWW: http://www.dothill.com
Attachment
Tom,
What is the status of the following patch:
http://postgresql.readysetnet.com/mhonarc/pgsql-cygwin/2001-04/msg00031.html
Is it possible to get it into 7.1.1?
Thanks,
Jason
--
Jason Tishler
Director, Software Engineering Phone: +1 (732) 264-8770 x235
Dot Hill Systems Corp. Fax: +1 (732) 264-8798
82 Bethany Road, Suite 7 Email: Jason.Tishler@dothill.com
Hazlet, NJ 07730 USA WWW: http://www.dothill.com
Jason Tishler <Jason.Tishler@dothill.com> writes: > What is the status of the following patch: > http://postgresql.readysetnet.com/mhonarc/pgsql-cygwin/2001-04/msg00031.html I recall voicing unhappiness about putting a definition of DLLIMPORT into sqlca.h, independent of the main one in c.h. Not sure if there's any good way around that, though. Including c.h would be a cure worse than the disease, probably (application namespace pollution and all that). The Makefile changes look OK to me but Peter E. would be more qualified to comment on them. regards, tom lane
Tom, On Thu, May 03, 2001 at 09:15:52AM -0400, Tom Lane wrote: > Jason Tishler <Jason.Tishler@dothill.com> writes: > > What is the status of the following patch: > > http://postgresql.readysetnet.com/mhonarc/pgsql-cygwin/2001-04/msg00031.html > > I recall voicing unhappiness about putting a definition of DLLIMPORT > into sqlca.h, independent of the main one in c.h. Not sure if there's > any good way around that, though. Including c.h would be a cure worse > than the disease, probably (application namespace pollution and all that). See the following to refresh your memory: On Fri, Apr 20, 2001 at 02:38:53PM -0400, Tom Lane wrote: > Jason Tishler <Jason.Tishler@dothill.com> writes: > > In the above mentioned URL, I suggested including c.h as another > > possibly better solution. However, I saw the following in ecpglib.h: > > /* > > * this is a small part of c.h since we don't want to leak all postgres > > * definitions into ecpg programs > > */ > > so I decided against including c.h in the first version of my patch. > > That's a fair point. Maybe it should be > > +#ifndef DLLIMPORT > +#ifdef __CYGWIN__ > +#define DLLIMPORT __declspec (dllimport) > +#else > +#define DLLIMPORT > +#endif /* __CYGWIN__ */ > +#endif /* DLLIMPORT */ The current version of my patch is just doing what you suggested. Are you no longer recommending this approach as the best compromise? > The Makefile changes look OK to me but Peter E. would be more qualified > to comment on them. Let's see what Peter has to say about the Makefile parts. Thanks, Jason -- Jason Tishler Director, Software Engineering Phone: +1 (732) 264-8770 x235 Dot Hill Systems Corp. Fax: +1 (732) 264-8798 82 Bethany Road, Suite 7 Email: Jason.Tishler@dothill.com Hazlet, NJ 07730 USA WWW: http://www.dothill.com
Jason Tishler <Jason.Tishler@dothill.com> writes:
> The current version of my patch is just doing what you suggested. Are you
> no longer recommending this approach as the best compromise?
I don't have a better idea ... but I thought I'd mention the point
again, just in case someone else can think of one.
>> The Makefile changes look OK to me but Peter E. would be more qualified
>> to comment on them.
> Let's see what Peter has to say about the Makefile parts.
Peter, if those changes look OK would you commit the patch? Or if
you're short of time today, let me know and I'll do it. Need to get
7.1.1 wrapped up ...
regards, tom lane
Tom Lane writes: > Peter, if those changes look OK would you commit the patch? Or if > you're short of time today, let me know and I'll do it. Need to get > 7.1.1 wrapped up ... Please commit and wrap ahead. IMO, the #ifdef DLLIMPORT should stay in sqlca.h, but the #ifdef __CYGWIN__ should probably outside of it (not inside). -- Peter Eisentraut peter_e@gmx.net http://funkturm.homeip.net/~peter
Peter Eisentraut <peter_e@gmx.net> writes:
> Please commit and wrap ahead. IMO, the #ifdef DLLIMPORT should stay in
> sqlca.h, but the #ifdef __CYGWIN__ should probably outside of it (not
> inside).
Huh? The macro has to get defined as *something* --- for non-CYGWIN
it must become defined as empty. Jason's code looks OK to me.
I applied the patch as it stood ... if you think you can improve the
style then go ahead ...
regards, tom lane
Tom, On Thu, May 03, 2001 at 12:15:46PM -0400, Tom Lane wrote: > I applied the patch as it stood ... Thanks! Jason -- Jason Tishler Director, Software Engineering Phone: +1 (732) 264-8770 x235 Dot Hill Systems Corp. Fax: +1 (732) 264-8798 82 Bethany Road, Suite 7 Email: Jason.Tishler@dothill.com Hazlet, NJ 07730 USA WWW: http://www.dothill.com