New issue
Advanced search Search tips

Issue 863460 link

Starred by 4 users

Issue metadata

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

Blocked on:
issue 863459


Show other hotlists

Hotlists containing this issue:
Chromium-Packagers


Sign in to add a comment

tools/gn/bootstrap/bootstrap.py should forward to tools/gn/build/gen.py

Project Member Reported by thomasanderson@chromium.org, Jul 13

Issue description

gen.py and bootstrap.py have very similar functions, so code should be deduplicated between them.  bootstrap.py should be a thin wrapper that forwards to gen.py.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 13

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/94f73c221e2b754bd7a8109fb2043bffa91e6f1e

commit 94f73c221e2b754bd7a8109fb2043bffa91e6f1e
Author: Tom Anderson <thomasanderson@chromium.org>
Date: Fri Jul 13 16:59:52 2018

Add tools/gn/bootstrap back

bootstrap.py does not exist in the gn.googlesource.com repo.  The script
gen/build.py is used instead.  However, there are some differences to
bootstrap.py: one obvious one is the name and location.  This breaks chromium
packagers relying on bootstrap.py, so this change adds the script back.  In the
long-term, it should forward to gen.py.

BUG= 859536 , 863459 , 863460 

Change-Id: I59ff81acb7f409d1975f99bcf0676e6103b2d7b9
Reviewed-on: https://chromium-review.googlesource.com/1136656
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574952}
[add] https://crrev.com/94f73c221e2b754bd7a8109fb2043bffa91e6f1e/tools/gn/bootstrap/OWNERS
[add] https://crrev.com/94f73c221e2b754bd7a8109fb2043bffa91e6f1e/tools/gn/bootstrap/bootstrap.py
[add] https://crrev.com/94f73c221e2b754bd7a8109fb2043bffa91e6f1e/tools/gn/bootstrap/build.ninja.template
[add] https://crrev.com/94f73c221e2b754bd7a8109fb2043bffa91e6f1e/tools/gn/bootstrap/build_aix.ninja.template
[add] https://crrev.com/94f73c221e2b754bd7a8109fb2043bffa91e6f1e/tools/gn/bootstrap/build_mac.ninja.template
[add] https://crrev.com/94f73c221e2b754bd7a8109fb2043bffa91e6f1e/tools/gn/bootstrap/build_vs.ninja.template

Blockedon: 863459
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 16

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/recipes-py/+/e396449f8434e3faf4151c36b5ad3f2377b80608

commit e396449f8434e3faf4151c36b5ad3f2377b80608
Author: Tom Anderson <thomasanderson@chromium.org>
Date: Mon Jul 16 23:47:38 2018

Add typechecking to file recipe module

While writing CL [1], I spent ~1hr debugging an issue that was caused by passing
a string to listdir() instead of a Path object.  The function appears to work,
but returns an odd result since it's returning str.join(str), which has a
different meaning than Path.join(str).  This CL adds typechecking to listdir()
and glob_paths() to ensure |source| is a Path.

R=iannucci
BUG= 863460 

Change-Id: Ia70833d80132e89726a242992cc7d8291fb945ff
Reviewed-on: https://chromium-review.googlesource.com/1139046
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>

[modify] https://crrev.com/e396449f8434e3faf4151c36b5ad3f2377b80608/README.recipes.md
[modify] https://crrev.com/e396449f8434e3faf4151c36b5ad3f2377b80608/recipe_modules/file/api.py

According to https://ci.chromium.org/buildbot/chromium.infra.cron/Build%20From%20Tarball/353 this is not enough: as noted in the chromium-packager mailing list gen.py itself is not part of the tarball.
My understanding was that the plan was to also add https://gn.googlesource.com/gn/ to the tarball and that's just not done yet. Not sure if there's some "get gn working for packagers again" umbrella bug that tracks everything that's missing.
Status: Started (was: Assigned)
c#6 that's the plan.

I have a CL sent out to do that:
https://chromium-review.googlesource.com/c/infra/infra/+/1138635
Just waiting to land it.
Project Member

Comment 8 by bugdroid1@chromium.org, Jul 18

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0fe5b65abadf19a1da7f8495d3bcdd48dba142b4

commit 0fe5b65abadf19a1da7f8495d3bcdd48dba142b4
Author: Tom Anderson <thomasanderson@chromium.org>
Date: Wed Jul 18 21:29:24 2018

Remove comments referencing bootstrap.py from base/BUILD.gn

The comments are no longer applicable.

BUG= 863460 
R=thakis

Change-Id: I29d89d18509f600aeac094fa577ec497377bcb93
Reviewed-on: https://chromium-review.googlesource.com/1142375
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576219}
[modify] https://crrev.com/0fe5b65abadf19a1da7f8495d3bcdd48dba142b4/base/BUILD.gn

This should hopefully be fixed now, but don't hesitate to post back here if the tarballs are still broken.
Status: Fixed (was: Started)
I'm not wild about keeping this stuff in //tools/gn just for the tarball's purposes.

I'm fine with it being there for now to unbreak things, but I'd like to get to a point where we can keep this stuff out of the tree. Hopefully that wouldn't actually require us to do separate GN packages, but that's a more feasible thing as well now that it is in a separate repo.
> This should hopefully be fixed now, but don't hesitate to post back here if the tarballs are still broken.

From https://ci.chromium.org/buildbot/chromium.infra.cron/Build%20From%20Tarball/356:

Traceback (most recent call last):
  File "/b/rr/tmpNgb6oc/w/build_dir/chromium-69.0.3495.2/tools/gn/bootstrap/bootstrap.py", line 96, in <module>
    sys.exit(main(sys.argv[1:]))
  File "/b/rr/tmpNgb6oc/w/build_dir/chromium-69.0.3495.2/tools/gn/bootstrap/bootstrap.py", line 81, in main
    os.path.join(BOOTSTRAP_DIR, 'last_commit_position.h'), gn_build_dir)
  File "/b/cipd_path_tools/lib/python2.7/shutil.py", line 144, in copy2
    copyfile(src, dst)
  File "/b/cipd_path_tools/lib/python2.7/shutil.py", line 96, in copyfile
    with open(src, 'rb') as fsrc:
IOError: [Errno 2] No such file or directory: '/b/rr/tmpNgb6oc/w/build_dir/chromium-69.0.3495.2/tools/gn/bootstrap/last_commit_position.h'
> I'm fine with it being there for now to unbreak things, but I'd like to get to a point where we can keep this stuff out of the tree.

Could you elaborate? If I understood you correctly, you're not a fan of either putting the GN sources into the tarball or making GN releases, so what would be the preferred way for packagers to build GN?
Status: Started (was: Fixed)
Reopening based on c#13
Project Member

Comment 16 by bugdroid1@chromium.org, Jul 19

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/581cb6f3627148447ce33788b828467b3446c25d

commit 581cb6f3627148447ce33788b828467b3446c25d
Author: Tom Anderson <thomasanderson@chromium.org>
Date: Thu Jul 19 19:01:34 2018

publish_tarball: Fix gn bootstrap path

Fixes this failure in the tarball builder:
https://ci.chromium.org/buildbot/chromium.infra.cron/Build%20From%20Tarball/356

BUG= 863460 
R=mmoss

Change-Id: I48c711e7ecec31e686ff1a28466a2b6f3d16e2a2
Reviewed-on: https://chromium-review.googlesource.com/1144059
Reviewed-by: Michael Moss <mmoss@chromium.org>
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>

[modify] https://crrev.com/581cb6f3627148447ce33788b828467b3446c25d/recipes/recipes/publish_tarball.py
[modify] https://crrev.com/581cb6f3627148447ce33788b828467b3446c25d/recipes/recipes/publish_tarball.expected/basic.json

Status: Fixed (was: Started)
The bot is still red: https://ci.chromium.org/buildbot/chromium.infra.cron/Build%20From%20Tarball/359

but in this case it looks like GN's being bootstrapped with the system libstdc++, which seems to be too old on the bot and doesn't have a lot of STL features //base expects to exist.
Status: Started (was: Fixed)
Project Member

Comment 20 by bugdroid1@chromium.org, Jul 23

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/d716a5c9698396c665d5e9625b6b026a492be955

commit d716a5c9698396c665d5e9625b6b026a492be955
Author: Tom Anderson <thomasanderson@chromium.org>
Date: Mon Jul 23 17:19:48 2018

Build From Tarball: build gn with Linux sysroot

This is needed to fix this failure on Build From Tarball:
https://ci.chromium.org/buildbot/chromium.infra.cron/Build%20From%20Tarball/359

BUG= 863460 
R=mmoss

Change-Id: I93f8c54c02ab795a1d496d8ade64ec781a1b28d4
Reviewed-on: https://chromium-review.googlesource.com/1145564
Reviewed-by: Michael Moss <mmoss@chromium.org>
Commit-Queue: Thomas Anderson <thomasanderson@chromium.org>

[modify] https://crrev.com/d716a5c9698396c665d5e9625b6b026a492be955/recipes/recipes/build_from_tarball.expected/basic.json
[modify] https://crrev.com/d716a5c9698396c665d5e9625b6b026a492be955/recipes/recipes/build_from_tarball.py

Status: Fixed (was: Started)
Third time's a charm
Status: Started (was: Fixed)
I think we're almost there :-) From https://ci.chromium.org/buildbot/chromium.infra.cron/Build%20From%20Tarball/368:

/b/rr/tmpxC6y6n/w/build_dir/chromium-70.0.3501.0/third_party/llvm-build/Release+Asserts/bin/clang++ -O3 -fdata-sections -ffunction-sections -Wl,--gc-sections -Wl,-strip-all --sysroot=/b/rr/tmpxC6y6n/w/build_dir/chromium-70.0.3501.0/tools/gn/build/../.linux-sysroot -static-libstdc++ -stdlib=libstdc++ -Wl,--as-needed -o gn -Wl,--start-group tools/gn/gn_main.o base.a gn_lib.a -Wl,--end-group -lgcc_s -lpthread
/usr/bin/ld: /b/rr/tmpxC6y6n/w/build_dir/chromium-70.0.3501.0/tools/gn/build/../.linux-sysroot/usr/lib/gcc/x86_64-linux-gnu/6/../../../x86_64-linux-gnu/crt1.o: unrecognized relocation (0x2a) in section `.text'
/usr/bin/ld: final link failed: Bad value
Cc: scottmg@chromium.org
Status: Fixed (was: Started)
Hopefully fixed on the bot now.  Though I'm not sure how Canonical is going to handle this with Trusty's Chromium.  We really ought to pull the libc++ sources in gn rather than pulling a sysroot.

+scottmg would you have any objections to replacing the sysroot with libc++ sources?  It would be less to download and would fix the issues on this bug.
Cc: phosek@chromium.org
https://bugs.chromium.org/p/chromium/issues/detail?id=855791#c39: "phosek@ is working on a way to statically link libc++ without needing to build it locally (which I'd rather avoid doing in the gn repo), but said that won't be rolled in from upstream clang for a while. So I'll stick with the linked CL for now, which should hopefully be OK." -- sounds like something like that is in the works. (phosek, is there some bug we can star to follow progress?)
Ah, yes that would be ideal.  Distros just need to start using our third_party clang, which so far all of them have refused to do.
our third_party clang is just clang from the future -- it's just upstream clang without any downstream patches. So distros will get our third_party clang as their regular clang in time :-)
That change have already landed in upstream (https://reviews.llvm.org/D45604), I'll be rolling out a Clang toolchain build with that change into goma and Fuchsia this week. Since GN bots currently use Fuchsia's Clang we can switch to libc++ after that lands.
> So distros will get our third_party clang as their regular clang in time :-)

Most distros are not rolling releases.  So Trusty for example will always have gcc 4.8 and clang whatever.
> I'm not sure how Canonical is going to handle this with Trusty's Chromium.

Canonical stopped issuing chromium updates on trusty three months ago, due to the increased difficulty of keeping it building on an old toolchain. See https://community.ubuntu.com/t/chromium-updates-on-trusty/5905 for details.
Huh weird, they could just use the bundled compiler and everything would work. ¯\_(ツ)_/¯

Sign in to add a comment