Joshua Hoblitt added a comment - 20/May/15 6:53 PM That was a poor description, the offender is `lib/SConscript`. https://github.com/lsst/afw/blob/master/lib/SConscript#L6-L9 eups is also imported by `src/math/detail/SConscript` https://github.com/lsst/afw/blob/master/src/math/detail/SConscript#L4-L16

Robert Lupton added a comment - 21/May/15 8:48 AM As Tim Jenness said in one of these tickets, this is using eups.productDir and Russell just provided a routine in utils to avoid this pattern. So please assign yourself these "no build without eups" tickets and fix them. I agree that it's a good idea to remove all use of eups to probe which packages are setup.

Russell Owen added a comment - 22/May/15 10:04 AM I asked Josh and he put the code changes in tickets/ DM-2636

Russell Owen added a comment - 22/May/15 10:30 AM Review of DM-2771 (work is on tickest/ DM-2636 ) For such a small change I'm afraid I have rather a lot to say. math/detail/SConscript: I feel your code swallows too many potential errors. I suggest putting only the getPackageDir in the try clause so you do not hide errors in the rest of the code. This looks like a good use of try/else to me; see example below. Picky point, but using "True" instead of "pass" in the except clause is unusual; it works but I find it confusing Using getPackageDir(packageName) to determine if a package is available is clearly a bit clumsy (the old behavior of returning None meant that one function could do two different jobs, but required a lot of wrappers when you wanted an instant failure if the package was not setup, and does not translate well to C++). Since you have to do it in several places I strongly suggest adding a new function to utils: isPackageAvailable(packageName). (The name could also be isPackageSetup, but that is a very EUPS-specific way to express the question). Here is my suggested alternative: if lsst.utils.isPackageAvailable('cuda_toolkit'): envCudaCommon = env.Clone() envCudaCommon.Append(NVCCFLAGS = ' -gencode=arch=compute_13,code=\\"sm_13,compute_13\\" ') envCudaCommon.Append(NVCCFLAGS = ' -gencode=arch=compute_20,code=\\"sm_20,compute_20\\" ') envConv = envCudaCommon.Clone() envConv.Append(NVCCFLAGS = ' -maxrregcount=60 ') envConv.Append(NVCCFLAGS = ' --ptxas-options=-v ') gpuObjConvolution = envConv.SharedObject("#/src/math/detail/convGPU", ["#src/math/detail/convGPU.cu"]) envWarp = envCudaCommon.Clone() envWarp.Append(NVCCFLAGS = ' -maxrregcount=60 ') envWarp.Append(NVCCFLAGS = ' --ptxas-options=-v ') gpuObjConvolution = envWarp.SharedObject('#src/math/detail/cudaLanczos', ['#src/math/detail/cudaLanczos.cu']) Or without isPackageAvailable: import lsst.utils try: lsst.utils.getPackageDir('cuda_toolkit') except Exception: pass else: envCudaCommon = env.Clone() envCudaCommon.Append(NVCCFLAGS = ' -gencode=arch=compute_13,code=\\"sm_13,compute_13\\" ') envCudaCommon.Append(NVCCFLAGS = ' -gencode=arch=compute_20,code=\\"sm_20,compute_20\\" ') envConv = envCudaCommon.Clone() envConv.Append(NVCCFLAGS = ' -maxrregcount=60 ') envConv.Append(NVCCFLAGS = ' --ptxas-options=-v ') gpuObjConvolution = envConv.SharedObject("#/src/math/detail/convGPU", ["#src/math/detail/convGPU.cu"]) envWarp = envCudaCommon.Clone() envWarp.Append(NVCCFLAGS = ' -maxrregcount=60 ') envWarp.Append(NVCCFLAGS = ' --ptxas-options=-v ') gpuObjConvolution = envWarp.SharedObject('#src/math/detail/cudaLanczos', ['#src/math/detail/cudaLanczos.cu']) tests/SConscript: The same issue applies about hiding too many errors. Do you need to set $AFWDATA_DIR? If so, a better comment about why would be appreciated (though I realize this is pre-existing code). If you are not sure it is needed please try without. afw/lib/SConscript: The same issue applies about hiding too many errors.

Joshua Hoblitt added a comment - 22/May/15 11:43 AM I wasn't aware of the try/else construct. I'll rework the PR shortly.

Joshua Hoblitt added a comment - 22/May/15 12:07 PM Russell Owen I've decided not to add another utility function to determine if a package is present. I'm concerned that it will end up being more plumbing to refactor in the near future. I don't know why AFWDATA_DIR is being setup in testsSConscript, I assume one of the test scripts depend on it. I think I've addressed your concerns about gratuitous exception hiding and repushed the branch.