afw can not be built without EUPS

XMLWordPrintable

Details

• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
None
• Team:
SQuaRE

Description

After manually removing the product dependencies, sconsUtils still attempts to import eups.

 $AFW_DIR=. scons  scons: Reading SConscript files ... Unable to import eups; guessing flavor Checking who built the CC compiler...error: no result CC is gcc version 4.8.3 Checking for C++11 support Checking whether the C++ compiler worksyes C++11 supported with '-std=c++11' Checking for C++ header file tr1/unordered_map... yes Setting up environment to build package 'afw'. Unable to import eups; guessing flavor Could not find EUPS product dir for 'afw'; using /home/vagrant/afw. Doxygen is not setup; skipping documentation build. ImportError: No module named eups:  File "/home/vagrant/afw/SConstruct", line 3:  scripts.BasicSConstruct("afw")  File "/home/vagrant/foo/python/lsst/sconsUtils/scripts.py", line 56:  versionModuleName, noCfgFile=noCfgFile)  File "/home/vagrant/foo/python/lsst/sconsUtils/scripts.py", line 106:  SCons.Script.SConscript(os.path.join(root, "SConscript"))  File "/usr/lib/scons/SCons/Script/SConscript.py", line 609:  return method(*args, **kw)  File "/usr/lib/scons/SCons/Script/SConscript.py", line 546:  return _SConscript(self.fs, *files, **subst_kw)  File "/usr/lib/scons/SCons/Script/SConscript.py", line 260:  exec _file_ in call_stack[-1].globals  File "/home/vagrant/afw/lib/SConscript", line 6:  import eups  Attachments Issue Links Activity Hide Joshua Hoblitt added a comment - 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 Show Joshua Hoblitt added a comment - 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 Hide Robert Lupton added a comment - 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. Show Robert Lupton added a comment - 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. Hide Russell Owen added a comment - I asked Josh and he put the code changes in tickets/DM-2636 Show Russell Owen added a comment - I asked Josh and he put the code changes in tickets/ DM-2636 Hide Russell Owen added a comment - 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.
Show
Russell Owen added a comment - 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.
Hide
Joshua Hoblitt added a comment -

I wasn't aware of the try/else construct. I'll rework the PR shortly.

Show
Joshua Hoblitt added a comment - I wasn't aware of the try/else construct. I'll rework the PR shortly.
Hide
Joshua Hoblitt added a comment -

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.

Show
Joshua Hoblitt added a comment - 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.

People

• Assignee:
Joshua Hoblitt
Reporter:
Joshua Hoblitt
Reviewers:
Russell Owen, Tim Jenness
Watchers:
Frossie Economou, Joshua Hoblitt, Robert Lupton, Russell Owen, Tim Jenness