Summary

Clearcase Popen problem on buggy python 2.7.x

Review Request #3994 — Created March 23, 2013 and submitted July 6, 2013, 1:57 p.m.

Information

Owner

delyn

Repository

Review Board

Branch

Bugs

Depends On

Reviewers

Groups

reviewboard

People

moonese

Description

Original issue was descibed in http://reviews.reviewboard.org/r/3804/
A console window would pop up every time popen is invoked on Windows 7 

Unfortunately, this fix has created a regression on other OSes
https://groups.google.com/forum/?fromgroups=#!topic/reviewboard-dev/hbrKN_p5Rvo

The goal of this update is to prevent Popen shell=true unless Windows 7 and python 2.7.x

Please comment it, do not whenever it is the best place/way to implement such _popen_shell=T/F

this can be merged from https://github.com/nanouck/reviewboard

Testing Done

on windows 7 and python 2.7.3
_popen_shell = True

on windows XP and python 2.7.3
_popen_shell = False

Issues

Description	From	Last Updated
Col: 1 W191 indentation contains tabs	reviewbot	March 23, 2013, 3:55 a.m.
Col: 1 E101 indentation contains mixed spaces and tabs	reviewbot	March 23, 2013, 3:55 a.m.
Col: 2 E128 continuation line under-indented for visual indent	reviewbot	March 23, 2013, 3:55 a.m.
Col: 1 W191 indentation contains tabs	reviewbot	March 23, 2013, 3:55 a.m.
Col: 1 E101 indentation contains mixed spaces and tabs	reviewbot	March 23, 2013, 3:55 a.m.
Col: 1 W191 indentation contains tabs	reviewbot	March 23, 2013, 3:55 a.m.
Col: 1 E101 indentation contains mixed spaces and tabs	reviewbot	March 23, 2013, 3:55 a.m.
Please add reference to the bugs.	BA bartek	March 23, 2013, 3:44 a.m.
Col: 1 E128 continuation line under-indented for visual indent	reviewbot	March 23, 2013, 3:56 a.m.
Col: 1 E128 continuation line under-indented for visual indent	reviewbot	March 23, 2013, 3:56 a.m.
Should be proper sentences, with capitalization and periods.	chipx86	April 8, 2013, 3:40 a.m.
I know we don't support Python 3.x yet, but we will in time. Do we have any idea about what …	chipx86	April 8, 2013, 3:41 a.m.
Is this a problem on Windows 8? Do we need to check for >= 7?	chipx86	April 8, 2013, 3:44 a.m.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/scmtools/clearcase.py
  Ignored Files:

reviewboard/scmtools/clearcase.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
reviewboard/scmtools/clearcase.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 E101 indentation contains mixed spaces and tabs
```
reviewboard/scmtools/clearcase.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 2
 E128 continuation line under-indented for visual indent
```
reviewboard/scmtools/clearcase.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
reviewboard/scmtools/clearcase.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 E101 indentation contains mixed spaces and tabs
```
reviewboard/scmtools/clearcase.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 W191 indentation contains tabs
```
reviewboard/scmtools/clearcase.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 1
 E101 indentation contains mixed spaces and tabs
```

reviewboard/scmtools/clearcase.py (Diff revision 2)

The issue has been resolved. Show all issues

Please add reference to the bugs.

delyn March 23, 2013, 3:04 a.m.

Bart, I would have liked to, but I cannot found them in http://code.google.com/p/reviewboard/issues :-(
You are the one who has reported regression, have you create such defect ?

bartek March 23, 2013, 3:08 a.m.

Oh, I thought about adding a similar explanation to the one in the review request with the link you mentioned: http://reviews.reviewboard.org/r/3804/ :)

DE

delyn March 23, 2013, 3:44 a.m.
```
sorry, I missunderstood it :-(
```

Change Summary:

update according to reviewbot and Bart comments

Description:

		Original issue was descibed in http://reviews.reviewboard.org/r/3804/
		A console window would pop up every time popen is invoked on Windows 7

		Unfortunately, this fix has created a regression on other OSes
		https://groups.google.com/forum/?fromgroups=#!topic/reviewboard-dev/hbrKN_p5Rvo

		The goal of this update is to prevent Popen shell=true unless Windows 7 and python 2.7.x

		Please comment it, do not whenever it is the best place/way to implement such _popen_shell=T/F

~		this can be merged from https://github.com/nanouck/reviewboard/commit/b04476b32dcef53054ebab66c94a909277bee792
	~	this can be merged from https://github.com/nanouck/reviewboard

Diff:

Revision 3 (+18 -6)

Show changes

reviewboard/scmtools/clearcase.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/scmtools/clearcase.py
  Ignored Files:

reviewboard/scmtools/clearcase.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 1
 E128 continuation line under-indented for visual indent
```
reviewboard/scmtools/clearcase.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 1
 E128 continuation line under-indented for visual indent
```

Change Summary:

pep8 E128

Diff:

Revision 4 (+18 -6)

Show changes

reviewboard/scmtools/clearcase.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/scmtools/clearcase.py
  Ignored Files:

Change Summary:: add moonese in reviewer list because he is the author of original #3804
People:: moonese

reviewboard/scmtools/clearcase.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Should be proper sentences, with capitalization and periods.
```
1. DE
  
  delyn April 8, 2013, 3:17 a.m.
  ok

reviewboard/scmtools/clearcase.py (Diff revision 4)

The issue has been resolved. Show all issues

I know we don't support Python 3.x yet, but we will in time. Do we have any idea about what versions there we may need to factor in?

delyn April 8, 2013, 3:17 a.m.

the issue is known for python 2.7.x only at the moment, so I'd rather to keep this check but can add a comment to track this information.

reviewboard/scmtools/clearcase.py (Diff revision 4)

The issue has been dropped. Show all issues

Is this a problem on Windows 8? Do we need to check for >= 7?

delyn April 8, 2013, 3:17 a.m.

currently we do not know if windows8 + python 2.7.x is affected, I have no windows 8 to check it. 

IMHO ">=7" is better because python 2.7.x was released before windows 7 and 8 and don't believe such issue could have been fixed

delyn April 8, 2013, 3:45 a.m.

finally I've just updated the comment because platform.release() return a string and "7", "8", and other version should be checked but I cannot test these strings (no such OSes) so I'd rather keep current test like that until someone report same issue with another known value of platform.release()

Change Summary:

update comment to clarify when issue happens

Diff:

Revision 5 (+21 -6)

Show changes

reviewboard/scmtools/clearcase.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/scmtools/clearcase.py
  Ignored Files:

Ship it!

Is there anything else that keeps this change from being submitted?

chipx86 June 6, 2013, 11:47 a.m.
```
Nope. We'll get it in for 1.7.10.
```

Status:: Completed
Change Summary:: Pushed to release-1.7.x (fbe1ad2)