Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
99473012:
Enable icu_use_data_file on Windows (Closed)
|
Created:
7 years ago by jungshik at Google Modified:
6 years, 11 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEnable icu_use_data_file on Windows
Set icu_use_data_file_flag to 1 on Windows in common.gypi and make the dependency on icudata conditional on 'icu_use_data_file_flag != 0' in common.gypi, base.gyp and net.gyp
Add icudtl.dat to the Windows build/archive/install file lists.
Load icudtl.dat from DIR_MODULE instead of DIR_EXE on Windows. (to fix bug 337116)
This also requires a change in third_party/icu/icu.gyp
( https://codereview.chromium.org/111723007/ ), which was rolled in by https://codereview.chromium.org/118313004/
BUG=72633, 337116
TEST=All windows builds work fine and there's no perf regression (start-up etc). Chrome installed via mini_installer runs fine (no issue with icu data file).
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246387
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246751
Patch Set 1 #Patch Set 2 : path name print fix on windows #Patch Set 3 : header file name fix #Patch Set 4 : add net.gyp #Patch Set 5 : add base.gyp #Patch Set 6 : add icu{uc,i18n} dependency to base_unittests #
Total comments: 8
Patch Set 7 : #Patch Set 8 : gyp files made to honor icu_use_data_file_flag #Patch Set 9 : icudtl.dat listed in win/server.rules #
Total comments: 1
Patch Set 10 : combine 2 conditions into one in base.gyp #
Total comments: 2
Patch Set 11 : icudt* optional / use wildcard in chrome.release #Patch Set 12 : rebased #Patch Set 13 : rebased again #
Total comments: 7
Patch Set 14 : '==1' => '!=0' per mark #
Total comments: 2
Patch Set 15 : fix an error in net.gyp and list icud*.d* explicitly #Patch Set 16 : fix a typo in net.gyp #
Total comments: 1
Patch Set 17 : indentation fix #Patch Set 18 : load icudtl.dat in DIR_MODULE instead of DIR_EXE on WIndows #
Messages
Total messages: 50 (0 generated)
Not yet ready for review because I'm still trying to find out why ninja is complaining about a rather unexpected circular dependency. icudt46l.dat -> obj\v8\tools\gyp\v8.actions_depends.stamp -> obj\v8\tools\gyp\v8_base.ia32.lib -> obj\v8\src\v8_base.ia32.accessors.obj -> obj\v8\tools\gyp\v8_base.ia32.compile_depends.stamp -> icudt46l.dat
On 2013/12/16 19:53:24, Jungshik Shin wrote: > Not yet ready for review because I'm still trying to find out why ninja is > complaining about a rather unexpected circular dependency. > > icudt46l.dat -> obj\v8\tools\gyp\v8.actions_depends.stamp -> > obj\v8\tools\gyp\v8_base.ia32.lib -> obj\v8\src\v8_base.ia32.accessors.obj -> > obj\v8\tools\gyp\v8_base.ia32.compile_depends.stamp -> icudt46l.dat Putting 'copies' for icudt*dat outside link_settings in icu.gyp seems to resolve the issue on Windows. ( https://codereview.chromium.org/111723007/ ).
With the icu-side change, this now works. Once the ICU CL goes in, I have to roll to that revision in this CL. In the meantime, can you take a look? Thanks.
Ping again when this is ready to go in for real, or if you want re-review for another reason. https://codereview.chromium.org/99473012/diff/90001/base/base.gyp File base/base.gyp (right): https://codereview.chromium.org/99473012/diff/90001/base/base.gyp#newcode704 base/base.gyp:704: '../third_party/icu/icu.gyp:icuuc', Why are you moving this? Seems like a no-op change that’s less sorted than it was before. If the order really is relevant, (1) I’d be surprised and (2) it definitely needs explanation. https://codereview.chromium.org/99473012/diff/90001/base/base.gyp#newcode798 base/base.gyp:798: # This is needed to trigger the dll copy step on windows. Move the comment into the condition it applies to. https://codereview.chromium.org/99473012/diff/90001/base/i18n/icu_util.cc File base/i18n/icu_util.cc (right): https://codereview.chromium.org/99473012/diff/90001/base/i18n/icu_util.cc#new... base/i18n/icu_util.cc:111: DLOG(ERROR) << "Couldn't mmap " << base::UTF16ToUTF8(data_path.value()); You can do this on all platforms, getting rid of the OS #ifdef and the new #include at the top of the file: data_path.AsUTF8Unsafe().value() Or, since this is just a DLOG, you can get rid of the filename altogether and just have it say "Couldn't mmap ICU data". That’s fine too, and none of the other DLOGs in this function try to print data_path even when they know it and it’s relevant. https://codereview.chromium.org/99473012/diff/90001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/99473012/diff/90001/net/net.gyp#newcode2184 net/net.gyp:2184: # This is needed to trigger the dll copy step on windows. Move into the conditon.
Thank you for the review. I addressed them all. In addition, I changed the Windows installer/test-related files to use icudtl.dat. I may have to use 'conditions' in installer gyp files. https://codereview.chromium.org/99473012/diff/90001/base/base.gyp File base/base.gyp (right): https://codereview.chromium.org/99473012/diff/90001/base/base.gyp#newcode704 base/base.gyp:704: '../third_party/icu/icu.gyp:icuuc', On 2013/12/17 22:12:02, Mark Mentovai wrote: > Why are you moving this? Seems like a no-op change that’s less sorted than it > was before. > > If the order really is relevant, (1) I’d be surprised and (2) it definitely > needs explanation. Sorry to get you 'alarmed'. That's just an accidental change. I'll revert. https://codereview.chromium.org/99473012/diff/90001/base/base.gyp#newcode798 base/base.gyp:798: # This is needed to trigger the dll copy step on windows. On 2013/12/17 22:12:02, Mark Mentovai wrote: > Move the comment into the condition it applies to. Done. https://codereview.chromium.org/99473012/diff/90001/base/i18n/icu_util.cc File base/i18n/icu_util.cc (right): https://codereview.chromium.org/99473012/diff/90001/base/i18n/icu_util.cc#new... base/i18n/icu_util.cc:111: DLOG(ERROR) << "Couldn't mmap " << base::UTF16ToUTF8(data_path.value()); On 2013/12/17 22:12:02, Mark Mentovai wrote: > You can do this on all platforms, getting rid of the OS #ifdef and the new > #include at the top of the file: > > data_path.AsUTF8Unsafe().value() > > Or, since this is just a DLOG, you can get rid of the filename altogether and > just have it say "Couldn't mmap ICU data". That’s fine too, and none of the > other DLOGs in this function try to print data_path even when they know it and > it’s relevant. Thanks. I went with AsUTF8Unsafe. https://codereview.chromium.org/99473012/diff/90001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/99473012/diff/90001/net/net.gyp#newcode2184 net/net.gyp:2184: # This is needed to trigger the dll copy step on windows. On 2013/12/17 22:12:02, Mark Mentovai wrote: > Move into the conditon. Done.
I agree, I think you want to make the changes in installer to respect icu_use_data_file_flag. Too bad not all of the spots look completely easy to fix.
Other than that, this looks good.
On 2013/12/18 21:29:07, Mark Mentovai wrote: > Other than that, this looks good.
On 2013/12/18 21:29:07, Mark Mentovai wrote: > Other than that, this looks good. Thanks for taking a look. It seems that I also have to list icudtl.dat in *isolate files. ( http://dev.chromium.org/developers/testing/isolated-testing/for-swes ). @maruel, what's the best way to deal with a conditional dependency in *isolate files? When icu_use_data_file_flag is set to 1, icudtl.dat is used on Windows,Linux,Mac, CrOS. Otherwise, icudt.dll is used on WIndows (on Linux and Mac, icudt is statically linked so that there's no separate dependency).
Anthony, with this change, we're gonna have 'icudtl.dat' in place of 'icudt.dll' on Windows. Do I need to do anything more for install/packaging?
On 2013/12/19 07:11:05, Jungshik Shin wrote: > Anthony, with this change, we're gonna have 'icudtl.dat' in place of 'icudt.dll' > on Windows. Do I need to do anything more for install/packaging? I missed two more files (other than *isolate files). I'll add them.
@maruel, is there any way to add files to *isolate with a condition other than OS==foo? (I thought I had asked this question yesterday, but apparently I didn't press the send button). icudtl.dat has to be added to a bunch of *isolate files if icu_use_data_file_flag is set to 1 (it's in build/common.gypi). In addition, icudt.dll has to be removed if icu_use_data_file_flag is 0 on Windows. Alternatively, is it ok to add files that are not present to *isolate files?
https://codereview.chromium.org/99473012/diff/140001/chrome/tools/build/win/s... File chrome/tools/build/win/server.rules (right): https://codereview.chromium.org/99473012/diff/140001/chrome/tools/build/win/s... chrome/tools/build/win/server.rules:12: AdditionalDependencies="$(SolutionDir)\tools\build\win\scan_server_dlls.py;$(OutDir)\chrome.exe;$(OutDir)\crash_reporter.exe;$(OutDir)\chrome.dll;$(OutDir)\locales\en-US.dll;$(OutDir)\icudtl.dat" Looks like I shouldn't add icudtl.dat here because it's not a dll. Can I leave this file alone even when icudt.dll is not present any more (when icu_use_data_file_flag is set to 1) or should I remove icudt.dll?
grt@chromium.org: Please review installer-related changes. BTW, how does chrome_frame deal with icu data loading? To minimize the change (loading from icudt.dll to icudtl.dat), I may as well disable this change for chrome_frame.
On 2013/12/20 10:40:10, Jungshik Shin wrote: > @maruel, is there any way to add files to *isolate with a condition other than > OS==foo? (I thought I had asked this question yesterday, but apparently I > didn't press the send button). > > icudtl.dat has to be added to a bunch of *isolate files if > icu_use_data_file_flag is set to 1 (it's in build/common.gypi). In addition, > icudt.dll has to be removed if icu_use_data_file_flag is 0 on Windows. > > Alternatively, is it ok to add files that are not present to *isolate files? Yes but the problem is that this would become copy-pasted into each .isolate file, something we'd like to avoid. So I'd recommend to create an icu.isolate that defines what is needed on each platform. Then have this file in the 'includes' section of each the .isolate that were including it. It seems that icudt.dll has been windows-specific up to now. To be able to use a new variable as a condition in .isolate file, you need to add it to build/isolate.gypi with the extra arguments: '--config-variable', 'icu_use_data_file_flag=<(icu_use_data_file_flag)' Sorry I understand this is a somewhat bigger change.
On 2013/12/20 12:11:38, Jungshik Shin wrote: > BTW, how does chrome_frame deal with icu data loading? To minimize the change > (loading from icudt.dll to icudtl.dat), I may as well disable this change for > chrome_frame. Until recently, Chrome Frame wasn't loading the dll and was therefore crashing when IDNs were navigated to. Oops. In M32, Chrome Frame calls base::i18n::InitializeICU() at load-time (see src/chrome_frame/chrome_tab.cc). Chrome Frame doesn't exist in M33, so don't put in any effort beyond making sure Chrome Frame still compiles and links. I'm in the process of removing the code, but at the moment we still need npchrome_frame to build. https://codereview.chromium.org/99473012/diff/160001/chrome/installer/mini_in... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/99473012/diff/160001/chrome/installer/mini_in... chrome/installer/mini_installer/chrome.release:31: icudtl.dat: %(VersionDir)s\ this won't do the right thing when icu_use_data_file_flag=0, will it? is it important to support that configuration? if so, you'll need to a bit more work, possibly modifying http://src.chromium.org/viewvc/chrome/trunk/src/chrome/tools/build/win/create... and its invocation for the 'installer_archive' target of mini_installer.gyp*. https://codereview.chromium.org/99473012/diff/160001/chrome/tools/build/win/F... File chrome/tools/build/win/FILES.cfg (right): https://codereview.chromium.org/99473012/diff/160001/chrome/tools/build/win/F... chrome/tools/build/win/FILES.cfg:98: 'filename': 'icudtl.dat', same comment here about not supporting a configuration. if both configurations are to be supported, you can make the two files optional here (search for 'optional' in this file for examples).
On 2013/12/20 14:22:27, M-A Ruel wrote: > On 2013/12/20 10:40:10, Jungshik Shin wrote: > > @maruel, is there any way to add files to *isolate with a condition other than > > OS==foo? (I thought I had asked this question yesterday, but apparently I > > didn't press the send button). > > > > icudtl.dat has to be added to a bunch of *isolate files if > > icu_use_data_file_flag is set to 1 (it's in build/common.gypi). In addition, > > icudt.dll has to be removed if icu_use_data_file_flag is 0 on Windows. > > > > Alternatively, is it ok to add files that are not present to *isolate files? > > Yes but the problem is that this would become copy-pasted into each .isolate > file, something we'd like to avoid. > > So I'd recommend to create an icu.isolate that defines what is needed on each > platform. Then have this file in the 'includes' section of each the .isolate > that were including it. It seems that icudt.dll has been windows-specific up to > now. > > To be able to use a new variable as a condition in .isolate file, you need to > add it to build/isolate.gypi with the extra arguments: > '--config-variable', 'icu_use_data_file_flag=<(icu_use_data_file_flag)' > > Sorry I understand this is a somewhat bigger change. Thanks a lot for the kind explanation. I saw your reply on the phone (I was ooo), but haven't replied in time. I'll make a separate CL to deal with *isolate file and use this CL as a switch to turn on 'icudt.dat' (as opposed to icudt.dll'). BTW, where should I put icu.isolate? Should it be in third_party/icu? In that case, can I use a relative path to refer to it from, say, chrome/chrome.isolate?
Thanks a lot for the kind explanation and sorry for the late reply. On 2013/12/20 15:15:14, grt wrote: > On 2013/12/20 12:11:38, Jungshik Shin wrote: > > BTW, how does chrome_frame deal with icu data loading? To minimize the change > > (loading from icudt.dll to icudtl.dat), I may as well disable this change for > > chrome_frame. > > Until recently, Chrome Frame wasn't loading the dll and was therefore crashing > when IDNs were navigated to. Oops. In M32, Chrome Frame calls > base::i18n::InitializeICU() at load-time (see src/chrome_frame/chrome_tab.cc). > Chrome Frame doesn't exist in M33, so don't put in any effort beyond making sure > Chrome Frame still compiles and links. I'm in the process of removing the code, > but at the moment we still need npchrome_frame to build. M33 branch was cut on Dec 17. So, I guess I don't have to worry about CF any more in the trunk (that is eventually going to be M34), right? > > https://codereview.chromium.org/99473012/diff/160001/chrome/installer/mini_in... > File chrome/installer/mini_installer/chrome.release (right): > > https://codereview.chromium.org/99473012/diff/160001/chrome/installer/mini_in... > chrome/installer/mini_installer/chrome.release:31: icudtl.dat: %(VersionDir)s\ > this won't do the right thing when icu_use_data_file_flag=0, will it? is it > important to support that configuration? if so, you'll need to a bit more work, > possibly modifying > http://src.chromium.org/viewvc/chrome/trunk/src/chrome/tools/build/win/create... > and its invocation for the 'installer_archive' target of mini_installer.gyp*. Thanks for the pointer. It does not seem hard to do that, but I'm not sure if it's worth doing. ( If using icudt.dat works well (scottmg wants to switch to icu*dat from icu*dll to prevent a 'memory frag'(?) issue on Windows XP. My motivation is to streamline the ICU maintenance/update). And, icu_use_data_file does not seem to be in the same league as others in create_installer_archive.py such as enable_hidpi, enable_touch_ui to have a separate section in chrome.release. How about using a wildcard 'icudt*.d*' to cover both 'icudtl.dat' and 'icudt.dat'? Would it be all right? I saw some entries with wildcard entries. > https://codereview.chromium.org/99473012/diff/160001/chrome/tools/build/win/F... > File chrome/tools/build/win/FILES.cfg (right): > > https://codereview.chromium.org/99473012/diff/160001/chrome/tools/build/win/F... > chrome/tools/build/win/FILES.cfg:98: 'filename': 'icudtl.dat', > same comment here about not supporting a configuration. if both configurations > are to be supported, you can make the two files optional here (search for > 'optional' in this file for examples). Thanks. That's easy. :-)
On 2014/01/02 23:45:41, Jungshik Shin wrote: > I'll make a separate CL to deal with *isolate file and use this CL as a switch > to turn on 'icudt.dat' (as opposed to icudt.dll'). > BTW, where should I put icu.isolate? Should it be in third_party/icu? In that > case, can I use a relative path to refer to it from, say, chrome/chrome.isolate? icu.isolate should be aside the corresponding .gyp file defining the target.
On 2014/01/03 00:59:36, Jungshik Shin wrote: > Thanks a lot for the kind explanation and sorry for the late reply. > > On 2013/12/20 15:15:14, grt wrote: > > On 2013/12/20 12:11:38, Jungshik Shin wrote: > > > BTW, how does chrome_frame deal with icu data loading? To minimize the > change > > > (loading from icudt.dll to icudtl.dat), I may as well disable this change > for > > > chrome_frame. > > > > Until recently, Chrome Frame wasn't loading the dll and was therefore crashing > > when IDNs were navigated to. Oops. In M32, Chrome Frame calls > > base::i18n::InitializeICU() at load-time (see src/chrome_frame/chrome_tab.cc). > > Chrome Frame doesn't exist in M33, so don't put in any effort beyond making > sure > > Chrome Frame still compiles and links. I'm in the process of removing the > code, > > but at the moment we still need npchrome_frame to build. > > > M33 branch was cut on Dec 17. So, I guess I don't have to worry about CF any > more in the trunk (that is eventually going to be M34), right? Correct. > > > > > > > https://codereview.chromium.org/99473012/diff/160001/chrome/installer/mini_in... > > File chrome/installer/mini_installer/chrome.release (right): > > > > > https://codereview.chromium.org/99473012/diff/160001/chrome/installer/mini_in... > > chrome/installer/mini_installer/chrome.release:31: icudtl.dat: %(VersionDir)s\ > > this won't do the right thing when icu_use_data_file_flag=0, will it? is it > > important to support that configuration? if so, you'll need to a bit more > work, > > possibly modifying > > > http://src.chromium.org/viewvc/chrome/trunk/src/chrome/tools/build/win/create... > > and its invocation for the 'installer_archive' target of mini_installer.gyp*. > > Thanks for the pointer. It does not seem hard to do that, but I'm not sure if > it's worth doing. ( > If using icudt.dat works well (scottmg wants to switch to icu*dat from icu*dll > to prevent a 'memory frag'(?) issue on Windows XP. My motivation is to > streamline the ICU maintenance/update). > > And, icu_use_data_file does not seem to be in the same league as others in > create_installer_archive.py such as enable_hidpi, enable_touch_ui to have a > separate section in chrome.release. > > How about using a wildcard 'icudt*.d*' to cover both 'icudtl.dat' and > 'icudt.dat'? Would it be all right? I saw some entries with wildcard entries. The only possible downside is both files accidentally ending up in the archive. I think the official builders do a clobber build, so this likely isn't an issue. Do you plan to remove support for the .dll once the .dat file is proven to work? If so, then LGTM. Please remove the wildcard once the .dat approach is proven.
On 2014/01/03 21:02:12, grt wrote: > On 2014/01/03 00:59:36, Jungshik Shin wrote: > > Thanks a lot for the kind explanation and sorry for the late reply. > > > > On 2013/12/20 15:15:14, grt wrote: > > > On 2013/12/20 12:11:38, Jungshik Shin wrote: > > > > BTW, how does chrome_frame deal with icu data loading? To minimize the > > change > > > > (loading from icudt.dll to icudtl.dat), I may as well disable this change > > for > > > > chrome_frame. > > > > > > Until recently, Chrome Frame wasn't loading the dll and was therefore > crashing > > > when IDNs were navigated to. Oops. In M32, Chrome Frame calls > > > base::i18n::InitializeICU() at load-time (see > src/chrome_frame/chrome_tab.cc). > > > Chrome Frame doesn't exist in M33, so don't put in any effort beyond making > > sure > > > Chrome Frame still compiles and links. I'm in the process of removing the > > code, > > > but at the moment we still need npchrome_frame to build. > > > > > > M33 branch was cut on Dec 17. So, I guess I don't have to worry about CF any > > more in the trunk (that is eventually going to be M34), right? > > Correct. > > > > > > > > > > > > > > https://codereview.chromium.org/99473012/diff/160001/chrome/installer/mini_in... > > > File chrome/installer/mini_installer/chrome.release (right): > > > > > > > > > https://codereview.chromium.org/99473012/diff/160001/chrome/installer/mini_in... > > > chrome/installer/mini_installer/chrome.release:31: icudtl.dat: > %(VersionDir)s\ > > > this won't do the right thing when icu_use_data_file_flag=0, will it? is it > > > important to support that configuration? if so, you'll need to a bit more > > work, > > > possibly modifying > > > > > > http://src.chromium.org/viewvc/chrome/trunk/src/chrome/tools/build/win/create... > > > and its invocation for the 'installer_archive' target of > mini_installer.gyp*. > > > > Thanks for the pointer. It does not seem hard to do that, but I'm not sure if > > it's worth doing. ( > > If usin강남구 강남대로 632 강남구 강남대로 632 강남구 강남대로 632 강남구 강남대로 632 g icudt.dat works well (scottmg wants to switch to icu*dat from icu*dll > > to prevent a 'memory frag'(?) issue on Windows XP. My motivation is to > > streamline the ICU maintenance/update). > > > > And, icu_use_data_file does not seem to be in the same league as others in > > create_installer_archive.py such as enable_hidpi, enable_touch_ui to have a > > separate section in chrome.release. > > > > How about using a wildcard 'icudt*.d*' to cover both 'icudtl.dat' and > > 'icudt.dat'? Would it be all right? I saw some entries with wildcard entries. > > > The only possible downside is both files accidentally ending up in the archive. > I think the official builders do a clobber build, so this likely isn't an issue. > Do you plan to remove support for the .dll once the .dat file is proven to work? > If so, then LGTM. Please remove the wildcard once the .dat approach is proven. Thank you for the review/lgtm and sorry for the late reply. Finally, the final prerequisite for this CL was landed yesterday. So, I'll land once I get the approval of other reviewers. @mark, @maruel, @scottmg, can you take a final look? Thanks
“error: old chunk mismatch” Try uploading again?
On 2014/01/16 22:20:29, Mark Mentovai wrote: > “error: old chunk mismatch” > > Try uploading again? Hmm... 'raw diff' is available, but 'view' led to that error. Anyway, I've just rebased to ToT and upload the CL again. (had to fight with 'git cl upload' ...)
The “old chunk mismatch” and unified inline diffs without side-by-side diffs happens when the base files aren’t uploaded. I’ve been getting a lot of this from my uploads lately: Uploading base file for chrome/common/url_constants.h Upload got a 500 response: 500 Upload got a 500 response: 500 Upload got a 500 response: 500 AppEngine is misbehaving and returned HTTP 500, again. Keep faith and retry or visit go/isgaeup. HTTP Error 500: Internal Server Error https://codereview.chromium.org/99473012/diff/570001/chrome/installer/mini_in... File chrome/installer/mini_installer.gyp (right): https://codereview.chromium.org/99473012/diff/570001/chrome/installer/mini_in... chrome/installer/mini_installer.gyp:230: }, { # else icu_use_data_file_flag == 1 technically != 0 https://codereview.chromium.org/99473012/diff/570001/chrome/installer/mini_in... File chrome/installer/mini_installer.gypi (right): https://codereview.chromium.org/99473012/diff/570001/chrome/installer/mini_in... chrome/installer/mini_installer.gypi:192: }, { # else icu_use_data_file_flag == 1 Same https://codereview.chromium.org/99473012/diff/570001/chrome/installer/mini_in... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/99473012/diff/570001/chrome/installer/mini_in... chrome/installer/mini_installer/chrome.release:28: icudt*.d*: %(VersionDir)s\ Doing a wildcard on the extension is a little weird. As long as files in this list are allowed to not exist, can you do icudt.dll and icudtl.dat separately? If not, is there a tighter way you can specify both files in one entry? bash would allow icudt*.{dat,dll} or even {icudt.dll,icudtl.dat}. I don’t know if that’s possible here.
LGTM with the suggested changes if possible.
https://codereview.chromium.org/99473012/diff/570001/chrome/installer/mini_in... File chrome/installer/mini_installer.gyp (right): https://codereview.chromium.org/99473012/diff/570001/chrome/installer/mini_in... chrome/installer/mini_installer.gyp:230: }, { # else icu_use_data_file_flag == 1 On 2014/01/17 19:03:59, Mark Mentovai wrote: > technically != 0 Done. https://codereview.chromium.org/99473012/diff/570001/chrome/installer/mini_in... File chrome/installer/mini_installer.gypi (right): https://codereview.chromium.org/99473012/diff/570001/chrome/installer/mini_in... chrome/installer/mini_installer.gypi:192: }, { # else icu_use_data_file_flag == 1 On 2014/01/17 19:03:59, Mark Mentovai wrote: > Same Done. https://codereview.chromium.org/99473012/diff/570001/chrome/installer/mini_in... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/99473012/diff/570001/chrome/installer/mini_in... chrome/installer/mini_installer/chrome.release:28: icudt*.d*: %(VersionDir)s\ On 2014/01/17 19:03:59, Mark Mentovai wrote: > Doing a wildcard on the extension is a little weird. As long as files in this > list are allowed to not exist, can you do icudt.dll and icudtl.dat separately? > If not, is there a tighter way you can specify both files in one entry? bash > would allow icudt*.{dat,dll} or even {icudt.dll,icudtl.dat}. I don’t know if > that’s possible here. I don't know either. :-) @grt, is it possible to tighten up the list as Mark suggested?
wtc@chromium.org: can you owner-approve the change in net.gyp? Thank you
https://codereview.chromium.org/99473012/diff/570001/chrome/installer/mini_in... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/99473012/diff/570001/chrome/installer/mini_in... chrome/installer/mini_installer/chrome.release:28: icudt*.d*: %(VersionDir)s\ On 2014/01/17 19:50:40, Jungshik Shin wrote: > On 2014/01/17 19:03:59, Mark Mentovai wrote: > > Doing a wildcard on the extension is a little weird. As long as files in this > > list are allowed to not exist, can you do icudt.dll and icudtl.dat separately? > > If not, is there a tighter way you can specify both files in one entry? bash > > would allow icudt*.{dat,dll} or even {icudt.dll,icudtl.dat}. I don’t know if > > that’s possible here. > > I don't know either. :-) > > @grt, is it possible to tighten up the list as Mark suggested? The script that reads this file uses Python's glob module, which I don't think supports the bash-y stuff Mark suggested. You could try listing the two files separately. It's possible that the script will happily ignore non-existent files.
Patch set 14: net/net.gyp LGTM.
Patch set 14: not LGTM. I found a bug. https://codereview.chromium.org/99473012/diff/640001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/99473012/diff/640001/net/net.gyp#newcode2341 net/net.gyp:2341: 'msvs_disabled_warnings': [4267, ], Jungshik: I am afraid that your change here is wrong. This MSVS compiler warning suppression should be independent of icu_use_data_file_flag.
Listing icudtl.dat and icudt.dll in chrome.release seem to work fine. I got no error while building mini_installer with both files listed. The list is fed to 7z and 7z didn't seem to care. https://codereview.chromium.org/99473012/diff/640001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/99473012/diff/640001/net/net.gyp#newcode2341 net/net.gyp:2341: 'msvs_disabled_warnings': [4267, ], On 2014/01/18 19:43:43, wtc wrote: > > Jungshik: I am afraid that your change here is wrong. This MSVS compiler warning > suppression should be independent of icu_use_data_file_flag. Thank you for catching it. Fixed it. Can you take another look?
Patch set 16: net/net.gyp LGTM. https://codereview.chromium.org/99473012/diff/730001/net/net.gyp File net/net.gyp (right): https://codereview.chromium.org/99473012/diff/730001/net/net.gyp#newcode2344 net/net.gyp:2344: 'msvs_disabled_warnings': [4267, ], Nit: the indentation of lines 2334-2344 probably should be two spaces more, to match the indentation of similar code in this file. (I have to say I would use the indentation you used, but it may be better to be consistent.)
On 2014/01/21 22:13:44, wtc wrote: > Patch set 16: net/net.gyp LGTM. > > https://codereview.chromium.org/99473012/diff/730001/net/net.gyp > File net/net.gyp (right): > > https://codereview.chromium.org/99473012/diff/730001/net/net.gyp#newcode2344 > net/net.gyp:2344: 'msvs_disabled_warnings': [4267, ], > > Nit: the indentation of lines 2334-2344 probably should be two spaces more, to > match the indentation of similar code in this file. (I have to say I would use > the indentation you used, but it may be better to be consistent.) Thank you. Fixed it. Adding to CQ.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jshin@chromium.org/99473012/800001
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, events_unittests, google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_unittests, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jshin@chromium.org/99473012/800001
On 2014/01/22 02:53:37, I haz the power (commit-bot) wrote: > Retried try job too often on win_rel for step(s) app_list_unittests, > ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests, > check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, > components_unittests, compositor_unittests, content_browsertests, > content_unittests, crypto_unittests, events_unittests, google_apis_unittests, > gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests, > media_unittests, mini_installer_test, nacl_integration, ppapi_unittests, > printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, > sync_unit_tests, telemetry_unittests, views_unittests > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... I think the following failure is not due to my CL. I'll add it to CQ once more. [20923/20925] RULE unit_tests_run: isolate_29fb0df6d7db4be43d73bead369449b3 unit_tests.isolate FAILED: E:\b\depot_tools\python276_bin\python.exe gyp-win-tool action-wrapper environment.x86 sync_integration_tests_run_target_isolate_71265b17974104fc91f615cb8e86fc35.fd13fc071dcd7d49564ed1c05ded379e.rsp ..\..\chrome ERROR net(486): Unable to open given url, https://isolateserver.appspot.com/content-gs/pre-upload/default-gzip?token=dg..., after 30 attempts. Exception: 500 Server Error: Internal Server Error ---------- Date: Wed, 22 Jan 2014 01:44:21 GMT Alternate-protocol: 443:quic Content-length: 323 Content-type: text/html; charset=UTF-8 Server: Google Frontend <html><head> <meta http-equiv="content-type" content="text/html;charset=utf-8"> <title>500 Server Error</title> </head> <body text=#000000 bgcolor=#ffffff> <h1>Error: Server Error</h1> <h2>The server encountered an error and could not complete your request.<p>Please try again in 30 seconds.</h2> <h2></h2> </body></html>
Message was sent while issue was closed.
Change committed as 246387
Message was sent while issue was closed.
On 2014/01/22 19:18:45, I haz the power (commit-bot) wrote: > Change committed as 246387 Hi Jungshik, While I am on Windows8 to debug Ash mode, I encountered a fail at the following line: content/app/content_main_runner.cc:735: CHECK(base::i18n::InitializeICU()); Reverting the CL fixed the issue for me, so even though I'm not 100% confident but this might be the issue. My workflow used was > ninja -C out\Debug chrome mini_installer > out\Debug\setup.exe --chrome --multi-installs --verbose-logging ...-> installer tries to launch the installed Chromium binary, and it fails. If I am mistaken, sorry for making a fuss.
Message was sent while issue was closed.
On 2014/01/23 09:29:01, Takayoshi Kochi wrote: > On 2014/01/22 19:18:45, I haz the power (commit-bot) wrote: > > Change committed as 246387 > > Hi Jungshik, > > While I am on Windows8 to debug Ash mode, I encountered a fail at the following > line: > > content/app/content_main_runner.cc:735: CHECK(base::i18n::InitializeICU()); > > Reverting the CL fixed the issue for me, so even though I'm not 100% confident > but > this might be the issue. > > My workflow used was > > > ninja -C out\Debug chrome mini_installer > > out\Debug\setup.exe --chrome --multi-installs --verbose-logging > ...-> installer tries to launch the installed Chromium binary, and it fails. > > If I am mistaken, sorry for making a fuss. Yeah, there's a bug report on mini_installer apparently not including/installing icudtl.dat even though the file is listed in what I believe to be necessary places. It's http://crbug.com/337116 I'll revert this CL and investigate. Thank you for the head-up.
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/139403006/ by jshin@chromium.org. The reason for reverting is: mini_installer somehow does not install icudtl.dat in the top product directory (alongside chrome.exe) leading to a failure in InitializeICU(). icudtl.dat is listed in both mini_installer.gypi and FILES.cfg for windows. .
Message was sent while issue was closed.
On 2014/01/23 17:42:49, Jungshik Shin wrote: > A revert of this CL has been created in > https://codereview.chromium.org/139403006/ by mailto:jshin@chromium.org. > > The reason for reverting is: mini_installer somehow does not install icudtl.dat > in the top product directory (alongside chrome.exe) leading to a failure in > InitializeICU(). > > icudtl.dat is listed in both mini_installer.gypi and FILES.cfg for windows. > . Having the data file alongside chrome.exe will be problematic. It should be in the version directory.
Message was sent while issue was closed.
On 2014/01/23 18:11:07, grt wrote: > On 2014/01/23 17:42:49, Jungshik Shin wrote: > > A revert of this CL has been created in > > https://codereview.chromium.org/139403006/ by mailto:jshin@chromium.org. > > > > The reason for reverting is: mini_installer somehow does not install > icudtl.dat > > in the top product directory (alongside chrome.exe) leading to a failure in > > InitializeICU(). > > > > icudtl.dat is listed in both mini_installer.gypi and FILES.cfg for windows. > > . > > Having the data file alongside chrome.exe will be problematic. It should be in > the version directory. Thank you for the chime-in. chrome.7z built locally has icudtl.dat in Chrome-bin/34.0.1799.0. I guess it means that icudtl.dat will be put in the version directory, right? OTOH, icu_utils.cc looks for it where chrome.exe is, which can explain this issue. Hmm, icudt.dll (before this CL) was also searched for where chrome.exe is, but icudt.dll is also installed in the version directory and InitializeICU() didn't have a trouble. Let's move this thread to http://crbug.com/337116.
Message was sent while issue was closed.
On 2014/01/23 19:20:48, Jungshik Shin wrote: > On 2014/01/23 18:11:07, grt wrote: > > On 2014/01/23 17:42:49, Jungshik Shin wrote: > > > A revert of this CL has been created in > > > https://codereview.chromium.org/139403006/ by mailto:jshin@chromium.org. > > > > > > The reason for reverting is: mini_installer somehow does not install > > icudtl.dat > > > in the top product directory (alongside chrome.exe) leading to a failure in > > > InitializeICU(). > > > > > > icudtl.dat is listed in both mini_installer.gypi and FILES.cfg for windows. > > > . > > > > Having the data file alongside chrome.exe will be problematic. It should be in > > the version directory. > > Thank you for the chime-in. > > chrome.7z built locally has icudtl.dat in Chrome-bin/34.0.1799.0. I guess it > means that icudtl.dat will be put in the version directory, right? OTOH, > icu_utils.cc looks for it where chrome.exe is, which can explain this issue. > Hmm, icudt.dll (before this CL) was also searched for where chrome.exe is, but > icudt.dll is also installed in the version directory and InitializeICU() didn't > have a trouble. > > Let's move this thread to http://crbug.com/337116. I was wrong about the search location of icudt.dll vs icudtl.dat. With icudt.dll (before the CL), it's searched for in DIR_MODULE while this CL looks for icudtl.dat in DIR_EXE. It works for Linux but not for Windows, I believe. In a local build (not installed through setup.exe / mini_installer.exe), DIR_EXE and DIR_MODULE would be the same, but when installed through setup/mini_installer, they'd be different. I'll make a new CL.
Message was sent while issue was closed.
On 2014/01/23 19:25:01, Jungshik Shin wrote: > On 2014/01/23 19:20:48, Jungshik Shin wrote: > > On 2014/01/23 18:11:07, grt wrote: > > > On 2014/01/23 17:42:49, Jungshik Shin wrote: > > > > A revert of this CL has been created in > > > > https://codereview.chromium.org/139403006/ by mailto:jshin@chromium.org. > > > > > > > > The reason for reverting is: mini_installer somehow does not install > > > icudtl.dat > > > > in the top product directory (alongside chrome.exe) leading to a failure > in > > > > InitializeICU(). > > > > > > > > icudtl.dat is listed in both mini_installer.gypi and FILES.cfg for > windows. > > > > . > > > > > > Having the data file alongside chrome.exe will be problematic. It should be > in > > > the version directory. > > > > Thank you for the chime-in. > > > > chrome.7z built locally has icudtl.dat in Chrome-bin/34.0.1799.0. I guess it > > means that icudtl.dat will be put in the version directory, right? OTOH, > > icu_utils.cc looks for it where chrome.exe is, which can explain this issue. > > Hmm, icudt.dll (before this CL) was also searched for where chrome.exe is, but > > icudt.dll is also installed in the version directory and InitializeICU() > didn't > > have a trouble. > > > > Let's move this thread to http://crbug.com/337116. > > I was wrong about the search location of icudt.dll vs icudtl.dat. With icudt.dll > (before the CL), it's searched for in DIR_MODULE while this CL looks for > icudtl.dat in DIR_EXE. It works for Linux but not for Windows, I believe. In a > local build (not installed through setup.exe / mini_installer.exe), DIR_EXE and > DIR_MODULE would be the same, but when installed through setup/mini_installer, > they'd be different. > > I'll make a new CL. I think I was able to update a committed CL and add the newest patch set to CQ (I remember doing that before), but somehow 'commit' checkbox can't be checked her with ps #18. I'll make a new CL. The only difference would be to use DIR_MODULE on Windows so that I'll TBR it. (I tested 'setup.exe --chrome --multi....' locally after testing *unittests).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jshin@chromium.org/99473012/1560001
Figured out how to 'resurrect' this CL. I have to reopen it before adding it to CQ again.
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jshin@chromium.org/99473012/1560001
Message was sent while issue was closed.
Change committed as 246751 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
