Closed
Bug 926252
Opened 11 years ago
Closed 10 years ago
versionbump.py should fail gracefully when git not found & suggest using --git <location_of_git>
Categories
(Testing :: Mozbase, defect)
Testing
Mozbase
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: emorley, Assigned: emorley)
Details
Attachments
(3 files, 1 obsolete file)
3.30 KB,
patch
|
k0scist
:
review+
|
Details | Diff | Splinter Review |
2.23 KB,
patch
|
k0scist
:
review+
|
Details | Diff | Splinter Review |
4.93 KB,
patch
|
k0scist
:
review+
|
Details | Diff | Splinter Review |
Currently: [/c/src/mozbase]$ git bash: git: command not found [/c/src/mozbase]$ python versionbump.py Checking for pypirc: c:/Users/Ed\.pypirc Traceback (most recent call last): File "versionbump.py", line 336, in <module> main() File "versionbump.py", line 123, in main cwd=here) File "c:\Python27\lib\subprocess.py", line 711, in __init__ errread, errwrite) File "c:\Python27\lib\subprocess.py", line 948, in _execute_child startupinfo) WindowsError: [Error 2] The system cannot find the file specified We should check for the presence of git at options.git_path and fail early if it is found.
Assignee | ||
Comment 1•11 years ago
|
||
* In later patches, more commands are going to use this wrapper, including ones that don't want the "Running foo..." printed to stdout. Therefore have moved the status messsages to outside of the wrapper, particularly since in many cases, a human-parsable explanation of what step we're at is more appropriate than the cmd passed. * Prints the cmd in a human readable format for failures rather than as a list. * All bar one caller uses a cwd of 'here' (and that caller explicitly passes it in), so let's make 'here' the default. * Moves the pypi status message to inside the loop, to make where failures occurred more obvious, given we're now not printing from the wrapper.
Attachment #816537 -
Flags: review?(jhammel)
Assignee | ||
Comment 2•11 years ago
|
||
Note: In the case of the first two uses of subprocess.Popen() being switched, we previously checked for contents of stderr as well as the return code. After switching, we do not. However, I don't believe the return code will be zero in cases that we care about anyway - and would rather avoid making call() more complicated if we can avoid it.
Attachment #816538 -
Flags: review?(jhammel)
Assignee | ||
Comment 3•11 years ago
|
||
* Makes the common git case fail early, and with a mention of --git * Ensures we handle other binaries more generically (or the git binary, but lack of permissions etc).
Attachment #816540 -
Flags: review?(jhammel)
Assignee | ||
Comment 4•11 years ago
|
||
There's loads more cleanup potential in this file, from things like exiting earlier in failure cases and not having to wait to search for all packages, through to not outputting a usage summary when we pass valid params, but I'll save those for later bugs.
Assignee | ||
Comment 5•11 years ago
|
||
From before: * In later patches, more commands are going to use this wrapper, including ones that don't want the "Running foo..." printed to stdout. Therefore have moved the status messsages to outside of the wrapper, particularly since in many cases, a human-parsable explanation of what step we're at is more appropriate than the cmd passed. * Prints the cmd in a human readable format for failures rather than as a list. * All bar one caller uses a cwd of 'here' (and that caller explicitly passes it in), so let's make 'here' the default. * Moves the pypi status message to inside the loop, to make where failures occurred more obvious, given we're now not printing from the wrapper. + Now: * Retains the print in the dry_run case, for extra confidence
Attachment #816537 -
Attachment is obsolete: true
Attachment #816537 -
Flags: review?(jhammel)
Attachment #816548 -
Flags: review?(jhammel)
Comment 6•11 years ago
|
||
Comment on attachment 816548 [details] [diff] [review] Part 1: subprocess.Popen() wrapper cleanup Review of attachment 816548 [details] [diff] [review]: ----------------------------------------------------------------- ::: versionbump.py @@ +30,2 @@ > if dry_run: > + print "Would have run: `%s`" % ' '.join(cmd) Not a fault of this patch, but it would be nice to have dry_run = False defined at the module global level to be slightly less confusing. Also, maybe better to use subprocess.list2cmdline vs the hand-join here. @@ +42,3 @@ > print stderr > + print "Return code: %s" % process.returncode > + raise CalledProcessError("Error running `%s`" % ' '.join(cmd)) again subprocess.list2cmdline @@ +318,2 @@ > for package in unrolled: > + print "Uploading to pypi: %s-%s" % (package, versions[package]) It would be nice to have this all printed first, but this is fine.
Attachment #816548 -
Flags: review?(jhammel) → review+
Comment 7•11 years ago
|
||
Comment on attachment 816538 [details] [diff] [review] Part 2: Switch remaining callers of subprocess.Popen() Review of attachment 816538 [details] [diff] [review]: ----------------------------------------------------------------- lgtm
Attachment #816538 -
Flags: review?(jhammel) → review+
Comment 8•11 years ago
|
||
Comment on attachment 816540 [details] [diff] [review] Part 3: Handle not finding git & others Review of attachment 816540 [details] [diff] [review]: ----------------------------------------------------------------- Nice! I'd go so far as to recommend using distutils.spawn.find_executable for all of our which needs (internally, we at least have which.which and mozrunner.utils.find_in_path) ::: versionbump.py @@ +1,5 @@ > #!/usr/bin/env python > > """bump mozbase versions""" > > +import distutils.spawn I didn't know about this one! Nice! @@ +35,5 @@ > kwargs.setdefault('cwd', here) > + try: > + process = subprocess.Popen(cmd, **kwargs) > + except OSError as e: > + raise Exception("Unable to run `%s`: %s" % (cmd[0], e.strerror)) Not sure how well this works on windows, but lgtm here.
Attachment #816540 -
Flags: review?(jhammel) → review+
Assignee | ||
Comment 9•10 years ago
|
||
Bug 949600 deleted versionbump.py, since it needs rewriting from scratch for hg. That's being handled in bug 974184, so this bug as it stands is WONTFIX, since the github repo is now frozen/depreciated.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•