New issue
Advanced search Search tips

Issue 749768 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug

Blocking:
issue 495204



Sign in to add a comment

/manifestinput: and /incremental interact in bad ways

Project Member Reported by thakis@chromium.org, Jul 27 2017

Issue description

https://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.
 
Cc: scottmg@chromium.org

Comment 2 by thakis@chromium.org, Jul 27 2017

Summary: /manifestinput: and /incremental interact in bad ways (was: /manifestinput: and /incremental interact in bad ways, apparently only in clang/win builds)
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.

Comment 3 by thakis@chromium.org, Jul 27 2017

Blocking: -82385

Comment 4 by thakis@chromium.org, Jul 27 2017

Reduced a bit:

  test("setup_unittests") {
    sources = [ "run_all_unittests.cc" ]  # empty file
    deps = [
      "//chrome/installer/mini_installer:unit_tests",
    ]
  }

Comment 5 by thakis@chromium.org, 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.

Comment 6 by thakis@chromium.org, 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?

Comment 7 by thakis@chromium.org, Jul 27 2017

Owner: thakis@chromium.org
Status: Started (was: Untriaged)
https://chromium-review.googlesource.com/590175 

Comment 8 by cpu@google.com, 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.

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
...

Comment 10 by cpu@google.com, Jul 27 2017

15K!!  we should fire someone :)

Yeah, lets ditch the merge thing. I will guess the impact is about 2K.
Project Member

Comment 11 by bugdroid1@chromium.org, 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

Project Member

Comment 12 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)

Sign in to add a comment