r74821 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r74820‎ | r74821 | r74822 >
Date:19:48, 15 October 2010
Author:platonides
Status:ok (Comments)
Tags:
Comment:
r71546 broke imagemagick usage in Windows, since cmd.exe takes the variable
names as the program name.
I'm just not exporting those variables here, but we need to support them in Windows, too.
We could use putenv() but that seems ugly (and risky). A better solution would be to have
wfShellExec() call proc_open() with a modified environment.

A POSIX shell is required to accept variable assignments before the command so we shouldn't
have problems on any UNIX-like system.
http://www.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_09_01
Modified paths:
  • /trunk/phase3/includes/media/Bitmap.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/media/Bitmap.php
——@@ -147,16 +147,13 @@
148148 }
149149 }
150150
 151+ // Use one thread only, to avoid deadlock bugs on OOM
 152+ $tempEnv = 'OMP_NUM_THREADS=1 ';
151153 if ( strval( $wgImageMagickTempDir ) !== '' ) {
152 - $tempEnv = 'MAGICK_TMPDIR=' . wfEscapeShellArg( $wgImageMagickTempDir ) . ' ';
153 - } else {
154 - $tempEnv = '';
 154+ $tempEnv .= 'MAGICK_TMPDIR=' . wfEscapeShellArg( $wgImageMagickTempDir ) . ' ';
155155 }
156156
157157 $cmd =
158 - $tempEnv .
159 - // Use one thread only, to avoid deadlock bugs on OOM
160 - 'OMP_NUM_THREADS=1 ' .
161158 wfEscapeShellArg( $wgImageMagickConvertCommand ) .
162159 // Specify white background color, will be used for transparent images
163160 // in Internet Explorer/Windows instead of default black.
——@@ -173,6 +170,12 @@
174171 " -depth 8 $sharpen" .
175172 " {$animation_post} " .
176173 wfEscapeShellArg( $this->escapeMagickOutput( $dstPath ) ) . " 2>&1";
 174+
 175+ if ( !wfIsWindows() ) {
 176+ // Assume we have a POSIX compliant shell which accepts variable assignments preceding the command
 177+ $cmd = $tempEnv . $cmd;
 178+ }
 179+
177180 wfDebug( __METHOD__.": running ImageMagick: $cmd\n" );
178181 wfProfileIn( 'convert' );
179182 $err = wfShellExec( $cmd, $retval );

Follow-up revisions

RevisionCommit summaryAuthorDate
r75476Provide a proper implementation for passing environment variables to wfShellE...platonides22:17, 26 October 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r71546Fixes for new ImageMagick:...tstarling08:48, 24 August 2010

Comments

#Comment by Hashar (talk | contribs)   09:39, 16 October 2010

Maybe we could use the windows SET command for environnent ? 

SET OMP_NUM_THREAD=1
#Comment by Platonides (talk | contribs)   22:18, 26 October 2010

To my surprise, we can. I added a full implementation in r75476.

#Comment by Hashar (talk | contribs)   06:36, 27 October 2010

Well done thanks!

Status & tagging log