Bug 701219 - swap os.system calls for idiomatic python calls #3

BYK · 2011-12-01T20:11:59Z

This changeset should fix the bug 701219 on Bugzilla

AutomatedTester · 2011-12-01T21:16:21Z

This looks good to me. I will leave it to Henrik to run and see if it works.

BYK · 2011-12-01T22:01:46Z

Added the fix for bug 700469 too by accident. Sorry about that.

whimboo · 2011-12-06T09:22:32Z

Thanks for those patches BYK. I have taken a look at those and I would kinda like to the the following remaining tasks done.

Would it be possible for you to separate the fixes for both bugs? Reason is that you have only worked on the Windows related setup script but left the ones for OS X and Linux alone. Both also would need the fixes for the logger and the removal of the os calls.
How do I enable the logging output for .info() messages? I don't see those on the command line until I configure the logger via basicConfig so it defaults to INFO.
We should better use logging.exception() for the code inside the download function. That will also dump the full stack. It's not such a problem here because the code is not that complex but a good to have in the log.

Otherwise it looks good, but I can't give response to its functionality because I haven't tested the patch yet.

BYK · 2011-12-06T10:37:34Z

whimboo,

I can create another branch for the logging task and move the commit there(which I should have done in the first place) though the files for other platforms are .sh files so I am confused. Should I convert them to python scripts too?
The default logging level is WARNING unfortunately and we have to configure the logging level via basicConf or something similar. We can actually redirect the logging to a file this way.
I changed it to exception though didn't committed it yet due to the combined commits problem in this branch. I'll do when we diced on the details.

Thanks for the review!

whimboo · 2011-12-12T22:04:42Z

Yes, please create a separate branch. Moving over the commit should be easy. Thanks. Also sorry for the confusion regarding other platforms. You are right and I missed that. We will convert the Bash scripts to Python scripts later on.

I would tend to set INFO as default logging level for this script. It's working fine for me. Redirecting to a file would be overhead IMHO.

Sorry that it has been taken bit longer but our mail system was down.

BYK · 2011-12-12T23:05:35Z

Okay, backed out the second commit. Will fix the issues you mentioned with logging in a seperate branch and create another pull-req ;)

PS: No problem with the late answer and the other scripts. ;)

whimboo · 2011-12-14T15:18:27Z

.gitignore

@@ -0,0 +1,3 @@
+.pydevproject
+.project
+*.py[co]


nit: please add an empty line at the end of the file.

whimboo · 2012-01-09T11:42:06Z

By end of last week I have had to push some other fixes to master, so it would require this pull request to be updated. Once it has been done I will review the patch. Sorry for the short delay.

whimboo · 2012-01-11T14:53:24Z

Burak, it would be great if we could merge your patch soon. Will you have time to update the pull request given the latest changes on master?

BYK · 2012-01-11T15:01:04Z

Henrik,

I'll try to update it today. If I can't, then we would have to wait until Friday. Does that work for you?

whimboo · 2012-01-11T15:02:08Z

Sure, sounds fine!

BYK · 2012-01-14T10:46:33Z

Made the merge, waiting for feedback! =)

whimboo · 2012-01-19T15:20:40Z

windows/build.py


 print "Copy template files into environment"
-os.system("xcopy /S /I /H %s %s" % (template_dir, env_dir))
+shutil.copytree(template_dir, env_dir, True)


This line always fails for me with the following error:

Copy template files into environment
Traceback (most recent call last):
File "Z:\data\code\mozmill-environment\windows\build.py", line 87, in

shutil.copytree(template_dir, env_dir, True)

File "C:\Python27\lib\shutil.py", line 174, in copytree
os.makedirs(dst)
File "C:\Python27\lib\os.py", line 157, in makedirs
mkdir(name, mode)
WindowsError: [Error 183] Cannot create a file when that file already exists: 'Z
:\data\code\mozmill-environment\windows\mozmill-env'

It happens because we already have created the env folder with the installation of MSYS.

whimboo · 2012-01-19T15:22:55Z

I have to wait with further testing until the above mentioned issue has been fixed. Also it looks like that this branch is still not on top of origin/master.

whimboo · 2012-01-30T17:09:43Z

We really want to have it fixed soon. Byk, if you do not have the time to work on this pull request, please make a note and we will finish up the patch. Thanks.

whimboo · 2012-03-01T11:19:52Z

Thanks Burak for the link. It looks good to me. Can you please ensure to update the pull request by merging the latest changes from master? It looks like its still behind because I can't automatically merge it.

Once it is done I will do another testrun. Sorry for the delay in my answer but I was on vacation.

BYK · 2012-03-01T11:26:05Z

@whimboo it was up to date and I also checked again to ensure it. GitHub sometimes cannot automatically merge even the simplest pull requests. I don't know how to make it so.

whimboo · 2012-03-01T11:39:35Z

@BYK something is clearly wrong and I can't even apply the patch to head locally

$ curl https://github.com/whimboo/mozmill-environment/pull/3.diff >_patch
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 7383 100 7383 0 0 5820 0 0:00:01 0:00:01 --:--:-- 11700
$ patch -p1 <_patch
patching file .gitignore
patching file windows/build.py
Hunk #1 succeeded at 28 (offset 5 lines).
Hunk #2 succeeded at 96 (offset 5 lines).
Hunk #3 succeeded at 116 (offset 5 lines).
Hunk #4 succeeded at 131 with fuzz 2 (offset 5 lines).
Hunk #5 FAILED at 150.
1 out of 5 hunks FAILED -- saving rejects to file windows/build.py.rej

I highly assume that this is the reason why github is not able to merge it.

BYK · 2012-03-01T11:46:03Z

I think you are right. Can you share that .rej file with me so I can investigate it further?

whimboo · 2012-03-01T12:04:05Z

I would propose that you do a clean clone and apply the patch file as given above. I have already deleted all those files from my local disk. sorry.

BYK · 2012-03-01T12:46:54Z

@whimboo I realized that I left some changes out. I merged them and tested the script. Hopefully it will merge without problems right now. If you would like to check it before merge, you can get a clone directly from my repo.

whimboo · 2012-03-01T13:06:36Z

windows/build.py


 print "Deleting pre-compiled Python modules and build folder"
-os.system("del /s /q %s\\*.pyc" % (python_dir))
-os.system("rd /s /q %s\\build" % (env_dir))
+remove_files(os.path.join(python_dir, "*.pyc"))


So I have checked the whole process and found a remaining issue with cleaning up the target folder from compiled scripts. With this change we do no longer remove those files. It results in:

-rw-r--r-- 1 henrik staff 12354881 Feb 15 15:18 1.5.9-win.zip
-rw-r--r-- 1 henrik staff 14285529 Mar 1 13:59 1.5.9-win-new.zip

So please fix the remove_files call.

whimboo · 2012-03-01T14:49:18Z

Looks good. Thanks for the updates.

Bug 701219 - swap os.system calls for idiomatic python calls

whimboo mentioned this pull request Dec 14, 2011

Overhaul logging #4

Closed

whimboo reviewed Dec 14, 2011
View reviewed changes

whimboo reviewed Jan 19, 2012
View reviewed changes

whimboo reviewed Mar 1, 2012
View reviewed changes

Fixed bug 701219 - swap os.system calls for idiomatic python calls

a9eb912

whimboo added a commit that referenced this pull request Mar 1, 2012

Merge pull request #3 from BYK/overhaul

87a9a44

Bug 701219 - swap os.system calls for idiomatic python calls

whimboo merged commit 87a9a44 into mozilla:master Mar 1, 2012

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 701219 - swap os.system calls for idiomatic python calls #3

Bug 701219 - swap os.system calls for idiomatic python calls #3

BYK commented Dec 1, 2011

AutomatedTester commented Dec 1, 2011

BYK commented Dec 1, 2011

whimboo commented Dec 6, 2011

BYK commented Dec 6, 2011

whimboo commented Dec 12, 2011

BYK commented Dec 12, 2011

whimboo Dec 14, 2011

whimboo commented Jan 9, 2012

whimboo commented Jan 11, 2012

BYK commented Jan 11, 2012

whimboo commented Jan 11, 2012

BYK commented Jan 14, 2012

whimboo Jan 19, 2012

whimboo commented Jan 19, 2012

whimboo commented Jan 30, 2012

whimboo commented Mar 1, 2012

BYK commented Mar 1, 2012

whimboo commented Mar 1, 2012

BYK commented Mar 1, 2012

whimboo commented Mar 1, 2012

BYK commented Mar 1, 2012

whimboo Mar 1, 2012

whimboo commented Mar 1, 2012

Bug 701219 - swap os.system calls for idiomatic python calls #3

Bug 701219 - swap os.system calls for idiomatic python calls #3

Conversation

BYK commented Dec 1, 2011

AutomatedTester commented Dec 1, 2011

BYK commented Dec 1, 2011

whimboo commented Dec 6, 2011

BYK commented Dec 6, 2011

whimboo commented Dec 12, 2011

BYK commented Dec 12, 2011

whimboo Dec 14, 2011

Choose a reason for hiding this comment

whimboo commented Jan 9, 2012

whimboo commented Jan 11, 2012

BYK commented Jan 11, 2012

whimboo commented Jan 11, 2012

BYK commented Jan 14, 2012

whimboo Jan 19, 2012

Choose a reason for hiding this comment

whimboo commented Jan 19, 2012

whimboo commented Jan 30, 2012

whimboo commented Mar 1, 2012

BYK commented Mar 1, 2012

whimboo commented Mar 1, 2012

BYK commented Mar 1, 2012

whimboo commented Mar 1, 2012

BYK commented Mar 1, 2012

whimboo Mar 1, 2012

Choose a reason for hiding this comment

whimboo commented Mar 1, 2012