Re: current state of bzr repository
On 03/27/2009 11:41 AM, Tsantilas Christos wrote: Amos Jeffries wrote: .. It's possibly version or system dependant. I've seen this myself, it was with g++, on either Solaris, CentOS, Ubuntu, or Debian, don't recall exactly which machine. But I have not used any non-gcc compilers for years. The one attempt to use Solaris native was a bust. I'm thinking to check if file contains only exactly debug.cc is probably cleaner and safer. That way we can ignore the cleaning process when compiler is already clean(er). My opinion is that the BuildPrefixInit() function is not safe. I believe that the best is to test the way the compiler uses the __FILE__ macro in configure script and if required use the strrchr function. But this is just my personal opinion... You are right. BuildPrefixInit has been broken since inception. As a short term fix, the assert() should be replaced with an if-statement. I think this was discussed a week or so ago on IRC but nobody had time to work on it, I guess. I have just committed the fix. Long term, more code needs to be written to cover more compilers and both in-tree and out-of-tree builds. I am not sure that code should go into configure.in because the BuildPrefixInit runs only once. It may be easier to place everything inside debug.cc. - The ufs aufs diskd does not link well. An src file like the src/FsReg.cc in my patch should added and fix a litle the libaufs.a libufs.a and other libraries (probably should included in one library) This is not possible at present. So long as admin have the option of specifying custom lists of FS (--enable-storeio) some of the FS may not exist at build time, and some custom ones we don't register in FsReg.cc may be added. The HAVE_* defines like those used for auth modules will have to be done for FsReg.cc first. Currently the libaufs.a library is only one code line: static StoreFSufsUFSSwapDir AufsInstance(DiskThreads, aufs); Maybe the best is to remove the libaufs and libdiskd libraries and move the declarations of AufsInstances DiskdInstance and UfsInstances in a FsReg.cc file. Always using the required HAVE_* defines Um, check with Alex. There are things in configure.in auto-linking that need to be considered carefully with those FS. OK I will try to implement something better and I will post it here. Thank you for working in this. I have commented on your patch separately. Alex.
Re: current state of bzr repository
Amos Jeffries wrote: - src/Debug.h The HERE macro is wrong. The original one was not bad Original one sometimes left strings with confidential build info in other peoples cache.log We spent a long while covering the possibilities to get the current one. If you'd like to trace down what cases are broken we can try for a better fix. OK. I am getting the follow: assertion failed: debug.cc:763: strstr(file,ThisFileNameTail)!=NULL The file points to debug.cc and the ThisFileNameTail points to src/debug.cc But is not the only problem I am seeing. In the case of the gcc the __FILE__ macro is the source file passed as argument. For example: if you use g++ -c debug.cc to compile your source file the __FILE__=debug.cc. if you use g++ -c /path/to/debug.cc then the __FILE__=/path/to/debug.cc. In our case now, from what I can understand the problem you are referring appeared on compilers other then gcc or when autotools for a reason use the full path name to compile the sources eg: g++ -c `pwd`/debug.cc Now imagine the case some files compiled using the g++ -c `pwd`/afile.cc and some others using the g++ -c afile.cc In the first case the __FILE__=/path/to/af.cc and in the second __FILE__=af.cc. In the second case in the SkipBuildPrefix function the line: return path+BuildPrefixLength; is problematic (maybe the BuildPrefixLength is 9 for example). I believe the SkipBuildPrefix is not safe as is. Maybe the best is to use strrchr to find the last /, if exists, and compute the basename using the result. - The ufs aufs diskd does not link well. An src file like the src/FsReg.cc in my patch should added and fix a litle the libaufs.a libufs.a and other libraries (probably should included in one library) This is not possible at present. So long as admin have the option of specifying custom lists of FS (--enable-storeio) some of the FS may not exist at build time, and some custom ones we don't register in FsReg.cc may be added. The HAVE_* defines like those used for auth modules will have to be done for FsReg.cc first. Currently the libaufs.a library is only one code line: static StoreFSufsUFSSwapDir AufsInstance(DiskThreads, aufs); Maybe the best is to remove the libaufs and libdiskd libraries and move the declarations of AufsInstances DiskdInstance and UfsInstances in a FsReg.cc file. Always using the required HAVE_* defines Also the DiskIO modules will need to be updated the same way and the DiskIO/DiskIOModules_gen.cc / pokeAllModules() hack will have to be removed at the same time. - The ntlm,basic digest schemes does not included in squid binary. A file like the src/AuthReg.cc in my patch should added Auth has the same problem as FS, but the HAVE_* hack kinkie added a while ago make your AuthReg an option now. Hm, I think we should carefully re-consider the use of globals now that we are explicitly forcing registration. For now okay. But the use of globals for reg needs a review I agree. What about the AuthReg.cc file? Should we wait other developers opinion? Regards, Christos
Re: current state of bzr repository
Amos Jeffries wrote: - src/Debug.h The HERE macro is wrong. The original one was not bad Original one sometimes left strings with confidential build info in other peoples cache.log We spent a long while covering the possibilities to get the current one. If you'd like to trace down what cases are broken we can try for a better fix. OK. I am getting the follow: assertion failed: debug.cc:763: strstr(file,ThisFileNameTail)!=NULL The file points to debug.cc and the ThisFileNameTail points to src/debug.cc But is not the only problem I am seeing. In the case of the gcc the __FILE__ macro is the source file passed as argument. For example: if you use g++ -c debug.cc to compile your source file the __FILE__=debug.cc. if you use g++ -c /path/to/debug.cc then the __FILE__=/path/to/debug.cc. In our case now, from what I can understand the problem you are referring appeared on compilers other then gcc or when autotools for a reason use the full path name to compile the sources eg: g++ -c `pwd`/debug.cc Now imagine the case some files compiled using the g++ -c `pwd`/afile.cc and some others using the g++ -c afile.cc In the first case the __FILE__=/path/to/af.cc and in the second __FILE__=af.cc. In the second case in the SkipBuildPrefix function the line: return path+BuildPrefixLength; is problematic (maybe the BuildPrefixLength is 9 for example). I believe the SkipBuildPrefix is not safe as is. Maybe the best is to use strrchr to find the last /, if exists, and compute the basename using the result. It's possibly version or system dependant. I've seen this myself, it was with g++, on either Solaris, CentOS, Ubuntu, or Debian, don't recall exactly which machine. But I have not used any non-gcc compilers for years. The one attempt to use Solaris native was a bust. I'm thinking to check if file contains only exactly debug.cc is probably cleaner and safer. That way we can ignore the cleaning process when compiler is already clean(er). - The ufs aufs diskd does not link well. An src file like the src/FsReg.cc in my patch should added and fix a litle the libaufs.a libufs.a and other libraries (probably should included in one library) This is not possible at present. So long as admin have the option of specifying custom lists of FS (--enable-storeio) some of the FS may not exist at build time, and some custom ones we don't register in FsReg.cc may be added. The HAVE_* defines like those used for auth modules will have to be done for FsReg.cc first. Currently the libaufs.a library is only one code line: static StoreFSufsUFSSwapDir AufsInstance(DiskThreads, aufs); Maybe the best is to remove the libaufs and libdiskd libraries and move the declarations of AufsInstances DiskdInstance and UfsInstances in a FsReg.cc file. Always using the required HAVE_* defines Um, check with Alex. There are things in configure.in auto-linking that need to be considered carefully with those FS. Also the DiskIO modules will need to be updated the same way and the DiskIO/DiskIOModules_gen.cc / pokeAllModules() hack will have to be removed at the same time. - The ntlm,basic digest schemes does not included in squid binary. A file like the src/AuthReg.cc in my patch should added Auth has the same problem as FS, but the HAVE_* hack kinkie added a while ago make your AuthReg an option now. Hm, I think we should carefully re-consider the use of globals now that we are explicitly forcing registration. For now okay. But the use of globals for reg needs a review I agree. What about the AuthReg.cc file? Should we wait other developers opinion? For now I'm okay with AuthReg.cc . Amos
Re: current state of bzr repository
Sorry Amos I did not found the time for the changes. The day does not have enough hours I will have time tomorrow.
Re: current state of bzr repository
Hi all, Currently the squid-trunk in bzr repository can not run, and can not compile at all. I am attaching a patch which shows some of the problems and proposes possible fixes: Hi Christos, looks like we have been working on the same bugs... - src/ip/IpIntercept.h file the PfInterception method has wrong definition - src/ip/IpIntercept.cc IpIntercept::NetfilterTransparent method uses an undefined parameter Would have committed for this one earlier but had other things interfere with the pre-commit build tests. That and the ACL Checklist are done just earlier... - src/ip/IpAddress.cc: has error in IpAddress::IpAddress constructor which causing infinite loop Doh. Thanks for catching that nasty one. Please apply as stand-alone patch. - src/Debug.h The HERE macro is wrong. The original one was not bad Original one sometimes left strings with confidential build info in other peoples cache.log We spent a long while covering the possibilities to get the current one. If you'd like to trace down what cases are broken we can try for a better fix. - The ufs aufs diskd does not link well. An src file like the src/FsReg.cc in my patch should added and fix a litle the libaufs.a libufs.a and other libraries (probably should included in one library) This is not possible at present. So long as admin have the option of specifying custom lists of FS (--enable-storeio) some of the FS may not exist at build time, and some custom ones we don't register in FsReg.cc may be added. The HAVE_* defines like those used for auth modules will have to be done for FsReg.cc first. Also the DiskIO modules will need to be updated the same way and the DiskIO/DiskIOModules_gen.cc / pokeAllModules() hack will have to be removed at the same time. - The ntlm,basic digest schemes does not included in squid binary. A file like the src/AuthReg.cc in my patch should added Auth has the same problem as FS, but the HAVE_* hack kinkie added a while ago make your AuthReg an option now. Hm, I think we should carefully re-consider the use of globals now that we are explicitly forcing registration. For now okay. But the use of globals for reg needs a review. - The ACLProxyAuth and ACLMaxUserIP acl classes (located under the src/auth directory) should initiated in AclRegs.cc file Agreed. Please commit your patch for those (AclRegs + auth/Acl*.cc changes) as stand-alone. ACLChecklist fix was applied earlier. I think squid-trunk should not stay more time in this state, some of the problems exist here for many days... always true :) Amos
