/manifestinput: and /incremental interact in bad ways |
|||||
Issue descriptionhttps://chromium-review.googlesource.com/c/588387 had three failures on win_clang that looked like: [1/1] LINK setup_unittests.exe setup_unittests.exe.pdb FAILED: setup_unittests.exe setup_unittests.exe.pdb C:/python_27_amd64/files/python.exe ../../build/toolchain/win/tool_wrapper.py link-wrapper environment.x64 False link.ex e /nologo /OUT:./setup_unittests.exe /PDB:./setup_unittests.exe.pdb @./setup_unittests.exe.rsp LINK : fatal error LNK1181: cannot open input file 'ild/win/common_controls.manifest' The .gn file and the rsp file look fine. I can reproduce this locally with args.gn of is_clang=true use_goma = true >autoninja -C out\gn setup_unittests -d keeprsp And only then patch in https://chromium-review.googlesource.com/c/588387 and run that again. Then the error will happen. On the next build, it won't. Deleting out\gn\setup_unittests.exe, then rebuilding it without the patch, and then with the patch again, makes it happen again. I'll try without clang next.
,
Jul 27 2017
I could repro this in a non-clang/win build as well. (the win_clang trybot builds "all" while the other doesn't; maybe setup_unittests only gets built if you build "all") [220/220] LINK setup_unittests.exe setup_unittests.exe.pdb FAILED: setup_unittests.exe setup_unittests.exe.pdb C:/python_27_amd64/files/python.exe ../../build/toolchain/win/tool_wrapper.py link-wrapper environment.x64 False link.ex e /nologo /OUT:./setup_unittests.exe /PDB:./setup_unittests.exe.pdb @./setup_unittests.exe.rsp LINK : fatal error LNK1181: cannot open input file 'ild/win/common_controls.manifest' ninja: build stopped: subcommand failed.
,
Jul 27 2017
,
Jul 27 2017
Reduced a bit:
test("setup_unittests") {
sources = [ "run_all_unittests.cc" ] # empty file
deps = [
"//chrome/installer/mini_installer:unit_tests",
]
}
,
Jul 27 2017
I believe this is related to these 500 bytes of savings here: https://cs.chromium.org/chromium/src/chrome/installer/mini_installer/mini_installer.cc?q=mini_installer.cc&sq=package:chromium&dr&l=22 // have the linker merge the sections, saving us ~500 bytes. #pragma comment(linker, "/MERGE:.rdata=.text") My setup is that I have out\gn\obj\chrome\installer\setup\setup_unittests.ninja open. I then remove more and more stuff from setup_unittests's deps, and then I run `autoninja -C out\gn setup_unittests -d keeprsp`, then delete the last two /manifestinput: flags from the .ninja file and run that again. If that errors, then I have the error, else I don't. With that, I reduced the one dep in comment 4 down to: source_set("unit_tests") { testonly = true sources = [ "mini_installer.cc", # contains jus that #pragma ] deps = [ "//base", ] } //base is probably just needed so that I don't have an empty .rdata section.
,
Jul 27 2017
This line was added by one cpu@ in commit d0ed6bd0a41a14457e894fccad578686441535e1 Author: cpu <cpu@9f7223c8-0a38-11dc-a625-d7732372afcb> Date: Tue Nov 20 22:08:39 2007 +0000 Second checking of the mini-installer. Contains the basic code to unpack the resources as files, uncompressing them and executing the real installer (setup.exe) long long ago. I'd say we sacrifice those 500 bytes in exchange for working /incremental links. How's that sound?
,
Jul 27 2017
https://chromium-review.googlesource.com/590175
,
Jul 27 2017
mini_installer.exe when invented was 4KB, this is one of many shortcuts taken to get a exe that small. You might be wondering why we cared. Well, because the omaha metainstaller was only 80KB (if my memory is correct) so back then the internet was just a series of pipes and we cared about each byte. I imagine that this is not the case anymore. It was pretty easy to regress on the size by simple having a better icon for the exe.
,
Jul 27 2017
You might be happy (?) to hear that it's actually still pretty small for a Chrome binary with the packed resources excluded (that's what pulls in the 4M setup.exe plus chrome.7z, right?)
c:\src\cr\src>git diff
diff --git a/chrome/installer/mini_installer/BUILD.gn b/chrome/installer/mini_installer/BUILD.gn
index 2afab1a..9b30660 100644
--- a/chrome/installer/mini_installer/BUILD.gn
+++ b/chrome/installer/mini_installer/BUILD.gn
@@ -222,7 +222,7 @@ template("generate_mini_installer") {
output_name = "mini_installer"
sources = [
"mini_installer_exe_main.cc",
- packed_files_rc_file,
+ #packed_files_rc_file,
]
# This target is special so we manually override most linker flags and
c:\src\cr\src>ninja -C out\Release mini_installer
ninja: Entering directory `out\Release'
...
c:\src\cr\src>dir out\release\mini_installer.exe
...
2017-07-27 02:48 PM 15,872 mini_installer.exe
...
,
Jul 27 2017
15K!! we should fire someone :) Yeah, lets ditch the merge thing. I will guess the impact is about 2K.
,
Jul 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/34d219ed15d7efa1e01cf832de018ff7f28eb995 commit 34d219ed15d7efa1e01cf832de018ff7f28eb995 Author: Nico Weber <thakis@chromium.org> Date: Thu Jul 27 23:33:42 2017 Make mini_installer ~500 bytes larger. Merging rdata and text breaks link.exe /incremental, is surprising for unexpecting bystanders, and 500 bytes is way below the daily fluctuation in binary size. Bug: 749768 Change-Id: I3c06990faf7dc5297cad0b4fd781225d52d31113 Reviewed-on: https://chromium-review.googlesource.com/590175 Reviewed-by: Gabriel Charette <gab@chromium.org> Commit-Queue: Nico Weber <thakis@chromium.org> Cr-Commit-Position: refs/heads/master@{#490107} [modify] https://crrev.com/34d219ed15d7efa1e01cf832de018ff7f28eb995/chrome/installer/mini_installer/mini_installer.cc
,
Jul 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9ad0916c64be00ef8ba7f7f6a9b3c78813e17f21 commit 9ad0916c64be00ef8ba7f7f6a9b3c78813e17f21 Author: Nico Weber <thakis@chromium.org> Date: Fri Jul 28 04:06:18 2017 win: Let linker append .manifest files instead of calling mt.exe This has a bunch of minor advantages: 1. It's less code 2. It's a build step less per windows_manifest 3. rc.exe is no longer explicitly required in the build, which means just distributing lld-link is enough on non-Windows hosts, we don't need to distribute an mt-like program (assuming lld-link internally doesn't need to shell out to mt). No intended behavior change. Bug: 495204, 749768 Change-Id: Ie079b82bc6217bc85942827f2c2843bb893d7186 Reviewed-on: https://chromium-review.googlesource.com/588387 Commit-Queue: Nico Weber <thakis@chromium.org> Reviewed-by: Scott Graham <scottmg@chromium.org> Cr-Commit-Position: refs/heads/master@{#490255} [modify] https://crrev.com/9ad0916c64be00ef8ba7f7f6a9b3c78813e17f21/build/config/win/manifest.gni [modify] https://crrev.com/9ad0916c64be00ef8ba7f7f6a9b3c78813e17f21/build/toolchain/win/tool_wrapper.py
,
Jul 28 2017
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by scottmg@chromium.org
, Jul 27 2017