Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8890072:
Yet another fix for concurrent calls to expand.exe. (Closed)
|
Created:
9 years ago by grt (UTC plus 2) Modified:
9 years ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionYet another fix for concurrent calls to expand.exe.
This time, try doing the work in uniquely named temp directories rather than blocking.
BUG=107291
TEST=Watch for build failures while extracting d3dx9_43.dll, D3DCompiler_43.dll, or xinput1_3.dll.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114290
Patch Set 1 : . #
Total comments: 3
Patch Set 2 : . #
Total comments: 6
Patch Set 3 : use shutil.rmtree #Patch Set 4 : removed TEMP/TMP override #
Total comments: 3
Patch Set 5 : ToT sync #Messages
Total messages: 12 (0 generated)
Gang: Please take a look at this new attempt to make cab extraction reliable. I don't have Python readability (or, possibly, writability), so feel free to comment the daylights out of it. Thanks. http://codereview.chromium.org/8890072/diff/2001/build/extract_from_cab.py File build/extract_from_cab.py (right): http://codereview.chromium.org/8890072/diff/2001/build/extract_from_cab.py#ne... build/extract_from_cab.py:48: os.rmdir(temp_dir) question for maruel: is shutil.rmtree(temp_dir, True) a better choice here?
This looks much better. Thanks. I don't have python readability either. I'll leave that to maruel. I had a look to see if there is a python module to extract things from cab files. Apparently it can only write them :( LGTM for my part. http://codereview.chromium.org/8890072/diff/2001/build/extract_from_cab.py File build/extract_from_cab.py (right): http://codereview.chromium.org/8890072/diff/2001/build/extract_from_cab.py#ne... build/extract_from_cab.py:39: # paving over any preexsing file. typo: preexisting
Thanks for the review. http://codereview.chromium.org/8890072/diff/2001/build/extract_from_cab.py File build/extract_from_cab.py (right): http://codereview.chromium.org/8890072/diff/2001/build/extract_from_cab.py#ne... build/extract_from_cab.py:39: # paving over any preexsing file. On 2011/12/13 19:17:09, apatrick_chromium wrote: > typo: preexisting Done.
http://codereview.chromium.org/8890072/diff/6001/build/extract_from_cab.py File build/extract_from_cab.py (right): http://codereview.chromium.org/8890072/diff/6001/build/extract_from_cab.py#ne... build/extract_from_cab.py:31: subprocess_env['TEMP'] = temp_dir Can you pinpoint which of the two it's using and only setting this one? I'd bet on TEMP. http://codereview.chromium.org/8890072/diff/6001/build/extract_from_cab.py#ne... build/extract_from_cab.py:48: os.rmdir(temp_dir) Yes, shutil.rmtree(temp_dir, True) is better.
maruel: PTAL, thanks. http://codereview.chromium.org/8890072/diff/6001/build/extract_from_cab.py File build/extract_from_cab.py (right): http://codereview.chromium.org/8890072/diff/6001/build/extract_from_cab.py#ne... build/extract_from_cab.py:31: subprocess_env['TEMP'] = temp_dir On 2011/12/13 19:35:54, Marc-Antoine Ruel wrote: > Can you pinpoint which of the two it's using and only setting this one? I'd bet > on TEMP. The Win7 version I tested uses neither; it uses the output directory given on the command line. I thought it would be more robust to set these as well since other versions could conceivably use TEMP/TMP. I'm comfortable removing them if they bring you displeasure. http://codereview.chromium.org/8890072/diff/6001/build/extract_from_cab.py#ne... build/extract_from_cab.py:48: os.rmdir(temp_dir) On 2011/12/13 19:35:54, Marc-Antoine Ruel wrote: > Yes, shutil.rmtree(temp_dir, True) is better. Done.
http://codereview.chromium.org/8890072/diff/6001/build/extract_from_cab.py File build/extract_from_cab.py (right): http://codereview.chromium.org/8890072/diff/6001/build/extract_from_cab.py#ne... build/extract_from_cab.py:31: subprocess_env['TEMP'] = temp_dir On 2011/12/13 19:47:24, grt wrote: > On 2011/12/13 19:35:54, Marc-Antoine Ruel wrote: > > Can you pinpoint which of the two it's using and only setting this one? I'd > bet > > on TEMP. > > The Win7 version I tested uses neither; it uses the output directory given on > the command line. I thought it would be more robust to set these as well since > other versions could conceivably use TEMP/TMP. I'm comfortable removing them if > they bring you displeasure. Well, if the code has no effect, I'd rather remove it. At worst, you could RDP into a XP slave with procmon just to verify.
Thanks; PTAL. http://codereview.chromium.org/8890072/diff/6001/build/extract_from_cab.py File build/extract_from_cab.py (right): http://codereview.chromium.org/8890072/diff/6001/build/extract_from_cab.py#ne... build/extract_from_cab.py:31: subprocess_env['TEMP'] = temp_dir On 2011/12/13 19:51:35, Marc-Antoine Ruel wrote: > On 2011/12/13 19:47:24, grt wrote: > > On 2011/12/13 19:35:54, Marc-Antoine Ruel wrote: > > > Can you pinpoint which of the two it's using and only setting this one? I'd > > bet > > > on TEMP. > > > > The Win7 version I tested uses neither; it uses the output directory given on > > the command line. I thought it would be more robust to set these as well > since > > other versions could conceivably use TEMP/TMP. I'm comfortable removing them > if > > they bring you displeasure. > > Well, if the code has no effect, I'd rather remove it. At worst, you could RDP > into a XP slave with procmon just to verify. Looks like TEMP/TMP aren't used in XP, so I've removed this.
lgtm http://codereview.chromium.org/8890072/diff/7002/build/extract_from_cab.py File build/extract_from_cab.py (right): http://codereview.chromium.org/8890072/diff/7002/build/extract_from_cab.py#ne... build/extract_from_cab.py:39: pass Can you add a warning print out here? Do you know any reason it'd fail?
http://codereview.chromium.org/8890072/diff/7002/build/extract_from_cab.py File build/extract_from_cab.py (right): http://codereview.chromium.org/8890072/diff/7002/build/extract_from_cab.py#ne... build/extract_from_cab.py:39: pass On 2011/12/13 20:17:10, Marc-Antoine Ruel wrote: > Can you add a warning print out here? Do you know any reason it'd fail? it fails if the file doesn't exist, which is actually the ordinary case. it'll also fail if permissions are wonky, which i would guess is out of the ordinary. a warning that didn't add noise would need to check that the remove failed and the file existed. do you think it's worth it? FWIW i found this pattern in other python scripts that remove a file that may or may not be present.
http://codereview.chromium.org/8890072/diff/7002/build/extract_from_cab.py File build/extract_from_cab.py (right): http://codereview.chromium.org/8890072/diff/7002/build/extract_from_cab.py#ne... build/extract_from_cab.py:39: pass On 2011/12/13 20:25:48, grt wrote: > On 2011/12/13 20:17:10, Marc-Antoine Ruel wrote: > > Can you add a warning print out here? Do you know any reason it'd fail? > > it fails if the file doesn't exist, which is actually the ordinary case. it'll > also fail if permissions are wonky, which i would guess is out of the ordinary. > a warning that didn't add noise would need to check that the remove failed and > the file existed. do you think it's worth it? FWIW i found this pattern in > other python scripts that remove a file that may or may not be present. That's fine, I don't really mind.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grt@chromium.org/8890072/7
Change committed as 114290 |
