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)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: emorley, Assigned: emorley)

Details

Attachments

(3 files, 1 obsolete file)

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.
* 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)
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)
* 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)
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.
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 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 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 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+
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.

Attachment

General

Created:
Updated:
Size: