Обсуждение: Adding flag LDFLAGS for compilation of regression tests
Hi all, I noticed that the regression tests of odbc do not take into account LDFLAGS at compilation, only CFLAGS actually. Having the possibility to use this flag is useful particularly when libodbc.so is in a customized path. Patch is attached. Regards, -- Michael
Вложения
On 03/14/2014 09:06 AM, Michael Paquier wrote: > Hi all, > > I noticed that the regression tests of odbc do not take into account > LDFLAGS at compilation, only CFLAGS actually. Having the possibility > to use this flag is useful particularly when libodbc.so is in a > customized path. Patch is attached. Hmm. The regression test Makefile really ought to pick up the same settings we use for the main makefile, so that if you do "./configure --with-unixodbc=...", the regression tests are automatically built against the same library. I'm not sure what's the best way to achieve that. The regression Makefile is currently completely separate from the automake system. We could add "SUBDIRS=test" into Makefile.am, and then you could do "make installcheck" from the top directory to run the regressions, and you could use the LDFLAGS and other variables set by automake. A problem with that is that the regression suite Makefile depends on PostgreSQL's pg_config to find the pg_regress program. I guess the proper solution would be to add an optional configure flag to provide a path to pg_config. If pg_config is not found, then you couldn't run the regression tests, but you could still build the driver without the PostgreSQL header files etc. - Heikki
On Fri, Mar 14, 2014 at 8:02 PM, Heikki Linnakangas <hlinnakangas@vmware.com> wrote: > Hmm. The regression test Makefile really ought to pick up the same settings > we use for the main makefile, so that if you do "./configure > --with-unixodbc=...", the regression tests are automatically built against > the same library. > > I'm not sure what's the best way to achieve that. The regression Makefile is > currently completely separate from the automake system. We could add > "SUBDIRS=test" into Makefile.am, and then you could do "make installcheck" > from the top directory to run the regressions, and you could use the LDFLAGS > and other variables set by automake. > > A problem with that is that the regression suite Makefile depends on > PostgreSQL's pg_config to find the pg_regress program. I guess the proper > solution would be to add an optional configure flag to provide a path to > pg_config. If pg_config is not found, then you couldn't run the regression > tests, but you could still build the driver without the PostgreSQL header > files etc. Even with that, don't we need first to move the automake process currently done on ./Makefile.am to another file? Like let's say Makefile.global.am, which contains all the variables set by automake. Then we create a new file ./Makefile at the root folder that includes Makefile.global. Something similar should be done with test/Makefile (inclusion of ./Makefile.global) and it would be able to use the values of CFLAGS, CDFLAGS and even PG_CONFIG that configure has set. Thoughts? -- Michael
On 03/17/2014 07:49 AM, Michael Paquier wrote: > On Fri, Mar 14, 2014 at 8:02 PM, Heikki Linnakangas > <hlinnakangas@vmware.com> wrote: >> Hmm. The regression test Makefile really ought to pick up the same settings >> we use for the main makefile, so that if you do "./configure >> --with-unixodbc=...", the regression tests are automatically built against >> the same library. >> >> I'm not sure what's the best way to achieve that. The regression Makefile is >> currently completely separate from the automake system. We could add >> "SUBDIRS=test" into Makefile.am, and then you could do "make installcheck" >> from the top directory to run the regressions, and you could use the LDFLAGS >> and other variables set by automake. >> >> A problem with that is that the regression suite Makefile depends on >> PostgreSQL's pg_config to find the pg_regress program. I guess the proper >> solution would be to add an optional configure flag to provide a path to >> pg_config. If pg_config is not found, then you couldn't run the regression >> tests, but you could still build the driver without the PostgreSQL header >> files etc. > > Even with that, don't we need first to move the automake process > currently done on ./Makefile.am to another file? Like let's say > Makefile.global.am, which contains all the variables set by automake. > Then we create a new file ./Makefile at the root folder that includes > Makefile.global. Something similar should be done with test/Makefile > (inclusion of ./Makefile.global) and it would be able to use the > values of CFLAGS, CDFLAGS and even PG_CONFIG that configure has set. Hmm, yes, something like that would be good. We can't easily restructure the main Makefile like that, though, because it's generated by automake. But we can still create a new Makefile.global file, with the same CFLAGS/LDFLAGS that the generated Makefile contains. Actually, we could just move test/Makefile to test/Makefile.in, and let configure set the LDFLAGS/CFLAGS variables directly in test/Makefile. I pushed a patch to do that. It doesn't do anything automatic about PG_CONFIG; it would still be nice to add a configure flag for that. PS. Have you set up a Windows build environment? I don't think anyone's put any effort in running the regression tests on Windows yet. If we could make that work somehow, that would be nice. - Heikki
On Mon, Mar 17, 2014 at 5:32 PM, Heikki Linnakangas <hlinnakangas@vmware.com> wrote: > On 03/17/2014 07:49 AM, Michael Paquier wrote: >> >> On Fri, Mar 14, 2014 at 8:02 PM, Heikki Linnakangas >> <hlinnakangas@vmware.com> wrote: >>> >>> Hmm. The regression test Makefile really ought to pick up the same >>> settings >>> we use for the main makefile, so that if you do "./configure >>> --with-unixodbc=...", the regression tests are automatically built >>> against >>> the same library. >>> >>> I'm not sure what's the best way to achieve that. The regression Makefile >>> is >>> currently completely separate from the automake system. We could add >>> "SUBDIRS=test" into Makefile.am, and then you could do "make >>> installcheck" >>> from the top directory to run the regressions, and you could use the >>> LDFLAGS >>> and other variables set by automake. >>> >>> A problem with that is that the regression suite Makefile depends on >>> PostgreSQL's pg_config to find the pg_regress program. I guess the proper >>> solution would be to add an optional configure flag to provide a path to >>> pg_config. If pg_config is not found, then you couldn't run the >>> regression >>> tests, but you could still build the driver without the PostgreSQL header >>> files etc. >> >> >> Even with that, don't we need first to move the automake process >> currently done on ./Makefile.am to another file? Like let's say >> Makefile.global.am, which contains all the variables set by automake. >> Then we create a new file ./Makefile at the root folder that includes >> Makefile.global. Something similar should be done with test/Makefile >> (inclusion of ./Makefile.global) and it would be able to use the >> values of CFLAGS, CDFLAGS and even PG_CONFIG that configure has set. > > > Hmm, yes, something like that would be good. We can't easily restructure the > main Makefile like that, though, because it's generated by automake. But we > can still create a new Makefile.global file, with the same CFLAGS/LDFLAGS > that the generated Makefile contains. As the code tree of odbc gets more complex, perhaps it might be worth doing it... Your patch seems to be enough for now though. > Actually, we could just move test/Makefile to test/Makefile.in, and let > configure set the LDFLAGS/CFLAGS variables directly in test/Makefile. > > I pushed a patch to do that. It doesn't do anything automatic about > PG_CONFIG; it would still be nice to add a configure flag for that. configure already sets PG_CONFIG, so we could do like in the patch attached and add a flag in test/Makefile.in similarly to CFLAGS and LDFLAGS. > PS. Have you set up a Windows build environment? I don't think anyone's put > any effort in running the regression tests on Windows yet. If we could make > that work somehow, that would be nice. No, I don't have that, but it is on my TODO list. I don't know precisely when I will be able to come to that, and I am still in the mood of giving a better test coverage first, like tests for catalog functions and deprecated functions. -- Michael
Вложения
On 03/17/2014 01:24 PM, Michael Paquier wrote: > On Mon, Mar 17, 2014 at 5:32 PM, Heikki Linnakangas > <hlinnakangas@vmware.com> wrote: >> Actually, we could just move test/Makefile to test/Makefile.in, and let >> configure set the LDFLAGS/CFLAGS variables directly in test/Makefile. >> >> I pushed a patch to do that. It doesn't do anything automatic about >> PG_CONFIG; it would still be nice to add a configure flag for that. > configure already sets PG_CONFIG, so we could do like in the patch > attached and add a flag in test/Makefile.in similarly to CFLAGS and > LDFLAGS. Hmm, PG_CONFIG is currently tied to the --with-libpq configure option, so that's only available if you build the driver with libpq. Would need to separate that into a separate option. (A slightly off-topic gripe: I just noticed that the #define constant is called "NOT_USE_LIBPQ", which makes for some odd double-negatives when most of the code blocks dependent on that are "#ifndef NOT_USE_LIBPQ". Would be better to have "USE_LIBPQ" and "#ifdef USE_LIBPQ". I think I'll go fix that..) >> PS. Have you set up a Windows build environment? I don't think anyone's put >> any effort in running the regression tests on Windows yet. If we could make >> that work somehow, that would be nice. > No, I don't have that, but it is on my TODO list. I don't know > precisely when I will be able to come to that, and I am still in the > mood of giving a better test coverage first, like tests for catalog > functions and deprecated functions. Ok, great. - Heikki
On 03/17/2014 03:18 PM, Heikki Linnakangas wrote: > On 03/17/2014 01:24 PM, Michael Paquier wrote: >> On Mon, Mar 17, 2014 at 5:32 PM, Heikki Linnakangas >> <hlinnakangas@vmware.com> wrote: >>> Actually, we could just move test/Makefile to test/Makefile.in, and let >>> configure set the LDFLAGS/CFLAGS variables directly in test/Makefile. >>> >>> I pushed a patch to do that. It doesn't do anything automatic about >>> PG_CONFIG; it would still be nice to add a configure flag for that. >> configure already sets PG_CONFIG, so we could do like in the patch >> attached and add a flag in test/Makefile.in similarly to CFLAGS and >> LDFLAGS. > > Hmm, PG_CONFIG is currently tied to the --with-libpq configure option, > so that's only available if you build the driver with libpq. Would need > to separate that into a separate option. Then again, if building with libpq, then there's no reason to *not* use the PG_CONFIG from configure. Committed a patch to do that. If you configure --without-libpq, then you still need to pass the path to PG_CONFIG manually at the command line when building the regression suite, i.e "cd test; make installcheck PG_CONFIG=<path>". But it's still an improvement that you don't need it when building with libpq, which is the default. - Heikki
On Mon, Mar 17, 2014 at 10:54 PM, Heikki Linnakangas <hlinnakangas@vmware.com> wrote: > On 03/17/2014 03:18 PM, Heikki Linnakangas wrote: >> Hmm, PG_CONFIG is currently tied to the --with-libpq configure option, >> so that's only available if you build the driver with libpq. Would need >> to separate that into a separate option. > > > Then again, if building with libpq, then there's no reason to *not* use the > PG_CONFIG from configure. Committed a patch to do that. If you configure > --without-libpq, then you still need to pass the path to PG_CONFIG manually > at the command line when building the regression suite, i.e "cd test; make > installcheck PG_CONFIG=<path>". But it's still an improvement that you don't > need it when building with libpq, which is the default. If the code tree of pgodbc gains in complexity in the future, something that I doubt will happen in a close future, we could still revisit that. But for now, what you have pushed is fine and enough IMO. So let's stick with that. -- Michael
On Tue, Mar 18, 2014 at 8:14 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Mon, Mar 17, 2014 at 10:54 PM, Heikki Linnakangas > <hlinnakangas@vmware.com> wrote: >> On 03/17/2014 03:18 PM, Heikki Linnakangas wrote: >>> Hmm, PG_CONFIG is currently tied to the --with-libpq configure option, >>> so that's only available if you build the driver with libpq. Would need >>> to separate that into a separate option. >> >> >> Then again, if building with libpq, then there's no reason to *not* use the >> PG_CONFIG from configure. Committed a patch to do that. If you configure >> --without-libpq, then you still need to pass the path to PG_CONFIG manually >> at the command line when building the regression suite, i.e "cd test; make >> installcheck PG_CONFIG=<path>". But it's still an improvement that you don't >> need it when building with libpq, which is the default. > If the code tree of pgodbc gains in complexity in the future, > something that I doubt will happen in a close future, we could still > revisit that. But for now, what you have pushed is fine and enough > IMO. So let's stick with that. By the way, could you push as well the patch attached? test/Makefile is now automatically generated by configure and it is not ignored in the code tree. Regards, -- Michael
Вложения
On 03/18/2014 07:51 AM, Michael Paquier wrote: > By the way, could you push as well the patch attached? test/Makefile > is now automatically generated by configure and it is not ignored in > the code tree. Thanks, fixed. (I added it to test/.gitignore instead) - Heikki
On Tue, Mar 18, 2014 at 4:09 PM, Heikki Linnakangas <hlinnakangas@vmware.com> wrote: > On 03/18/2014 07:51 AM, Michael Paquier wrote: >> >> By the way, could you push as well the patch attached? test/Makefile >> is now automatically generated by configure and it is not ignored in >> the code tree. > > > Thanks, fixed. (I added it to test/.gitignore instead) Yep, makes sense. -- Michael