-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prevent conflicting AMBuild installations. #75
Conversation
This detects older AMBuild installs that may have been installed with distutils, and forces their removal.
import ambuild2.util | ||
except: | ||
sys.exit(2) | ||
if getattr(ambuild2.util, 'INSTALLED_BY_PIP_OR_SETUPTOOLS', False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional A: Use a sentinel object
sentinel = object() # Plain object for minimal memory usage.
if getattr(ambuild2.util, 'INSTALLED_BY_PIP_OR_SETUPTOOLS', sentinel) is not sentinel:
Option B: Catch "AttributeError"
try:
obj = getattr(ambuild2.util, 'INSTALLED_BY_PIP_OR_SETUPTOOLS')
except AttributeError:
pass
else:
sys.exit(2)
Option A is less weird looking, Option B is more "pythonic"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sanity check:
Python 2.7+ confirms "object()" results in new objects, not a cached object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to keep this simple - Python is... not my first choice anymore, and maintaining AMBuild is hard. I have to remember a lot of quirks. It should have been written in Go, but that wasn't around when AMBuild was needed. Anyway, "True" is a lot more clear than "object()", especially given the boolean phrasing of the variable name.
sys.stderr.write("do this by inspecting the following locations and removing\n") | ||
sys.stderr.write("any ambuild folders:\n\n") | ||
for path in sys.path[1:]: | ||
candidates = [ os.path.join(path, "ambuild"), os.path.join(path, "ambuild2") ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicks:
- Use a tuple, candidates is implied to be immutable.
- A generator expression would also suffice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, I try to be consistent in using lists over tuples. Tuples are quirky. It's not clear why most code would care about immutability.
Other than the changes I suggested (Nitpick review not included), I'm fine with doing this @dvander. |
Thanks for taking a look, hopefully no one runs into any problems. |
@dvander It seems you're using multiprocessing wrong according to my local Python install:
|
According to the docs, you need to insert a 'freeze_support()' call before creating the Process object. |
This detects older AMBuild installs that may have been installed with
distutils, and forces their removal.