New issue
Advanced search Search tips

Issue 742527 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 3
Type: Bug



Sign in to add a comment

GetPlatforms in build/mac_toolchain.py isn't quite right

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

Issue description

build/mac_files/Xcode.app was out-of-date on my machine.

It turned out to be due to me having `target_os = ['ios']` in my .glient, which causes mac_toolchain.py to believe that I'm _only_ interested in iOS builds, while the sematics of that field are that I'm _also_ interested in iOS builds. (For example, target_os = ['android', 'fuchsia'] on Linux means you'll get a checkout that can do linux, android, and fuchsia builds.)

So I tried to hack up mac_toolchain.py to use what's in there in addition to 'mac' instead of replacement, and since there's a loop over the results of GetPlatforms() further down I thought it might work:

$ git diff
diff --git a/build/mac_toolchain.py b/build/mac_toolchain.py
index 534f9d1982be..33933b6932fa 100755
--- a/build/mac_toolchain.py
+++ b/build/mac_toolchain.py
@@ -57,14 +57,14 @@ def PlatformMeetsHermeticXcodeRequirements(target_os):
 
 
 def GetPlatforms():
-  default_target_os = ["mac"]
+  target_os = set(["mac"])
   try:
     env = {}
     execfile(GCLIENT_CONFIG, env, env)
-    return env.get('target_os', default_target_os)
+    target_os |= set(env.get('target_os', default_target_os))
   except:
     pass
-  return default_target_os
+  return target_os
 
 
 def ReadStampFile(target_os):


However, unlike on Windows, build/mac_files doesn't put the toolchain output in a subdirectory with the hash, so the two toolchains clobber each other as far as I can tell. That's not good.


Things we might want to do:
1. Add toolchain hash to dir, so we end up with build/mac_files/8e2002/Xcode.app and build/mac_files/8c1002/Xcode.app
2. Use same toolchain for mac and ios; the ios toolchain needs a mac toolchain for host stuff anyhow (not sure about this one)
 
Cc: dpranke@chromium.org justincohen@chromium.org
This is currently the mechanism that builders use to figure out whether to pull down the iOS or macOS toolchain. Since this a DEPs step, the only other way to pass information through is to use an ENV variable. 

To support both the developer and bot use case, we need to support the following configurations: no toolchain, just mac/iOS, both mac & iOS. 

So we probably want ENV variables: FORCE_MAC_TOOLCHAIN, FORCE_IOS_TOOLCHAIN [I'm sure dpranke will be ecstatic], and allow up to two toolchain installs [for mac and iOS]. 

For develoers, if neither ENV variable is set, then we download by default the mac toolchain, but *also* download the iOS toolchain if targetting iOS.
Until Apple's ibtool stops causing problems, we can't use hermetic for developers.  Running Xcode locally plus building with a hermetic will cause the world to implode.  For the time being iOS hermetic is bots only. 
In that case, the bots should get the iOS hermetic toolchain using FORCE_IOS_TOOLCHAIN, and that way developers will *always* pull the macOS toolchain.

Comment 4 by tapted@chromium.org, Jul 21 2017

Owner: justincohen@chromium.org
Status: Assigned (was: Untriaged)
[mac triage] taking out of triage

Comment 5 by costan@google.com, Aug 2 2017

Just got bit by this today, because I picked up https://crrev.com/c/570459 after a gclient sync, on a (Google) corp MacOS machine. I had target_os = [ 'ios' ] in my .gclient, per the iOS build instructions, so I wasn't getting the new toolchain.

My workaround was to set target_os to [ 'mac', 'ios' ]. After another gclient sync, I'm happily building Chromium again.

Up until now, I didn't realize we're using a hermetic toolchain for corp machines. The error messages were a bit confusing, because I do have my own Xcode with the 10.12 SDK installed. Fortunately, the error messages did contain build/mac/find_sdk.py, which was enough to start figuring things out. 

This might be worth a PSA?
Or we could just fix this bug and make things Just Work, i.e. erikchen, can you implement comment 3?
Chatted with Justin, this is on his to-do list, but low priority.
In that case, a brief message to the right mailing list could save some eng time. Note that this doesn't only impact folks who are actively working on iOS -- I ended up with target_os = [ 'ios' ] because I needed to figure out a build failure at some point.
I'm lost here. Can someone re-state which combinations do or don't work, and what the proposed fixes are?
The fix here is a one-line change (or deleting more than one line). Not fixing this is disruptive for everyone who works on both chrome/mac and chrome/ios. Can we please just fix?

dpranke:
If you have `target_os = ['ios']` in your .gclient, then we don't download the hermetic mac toolchain (which kind of breaks the mac build). (We do download the hermetic ios toolchain instead, which currently isn't used for anything.)
This isn't a 1-line fix, since we need to fix all the bots that currently rely on this logic to get the IOS toolchain.
we could hack in a fix by detecting if this looks like a corp device, and if so, forcing the macOS toolchain.

This *might* have problems if there are any stray corp-images floating around in our fleet. This would also prevent users from using the IOS hermetic toolchain, but maybe there aren't any?
Doesn't that just make it 3 lines then though? In the recipe, check for ios, if set, set env var, in script check for env var?
when you say that using the hermetic toolchain doesn't work for developers, are you really saying *ios developers* who need to be running xcode interactively? 

If I just want to build and run tests locally without running xcode, shouldn't the hermetic toolchain work just as well for me locally as it would on a bot?
The trick is identifying the necessary and sufficient recipes to set the env var. And then dealing with fallout when you inevitably over or under-specify the appropriate recipes. 

I've put together a 3-line hack to deal with this problem until Justin has time to plumb through the ENV variable for iOS:
https://chromium-review.googlesource.com/c/598776
Project Member

Comment 16 by bugdroid1@chromium.org, Aug 2 2017

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

commit a211039cca8b7afe0edc753d051772a5ec9d63d7
Author: erikchen <erikchen@chromium.org>
Date: Wed Aug 02 21:11:18 2017

Temporary workaround for macOS hermetic toolchain not being used.

Users who specify an 'ios' solution in .gclient will pull the iOS toolchain.
This is used by 'ios' bots to pull the toolchain. But macOS developers who
happen to specify an 'ios' solution will also pull the iOS toolchain. This CL is
a temporary hack to work around this: if the machine appears to be a corp
machine, always pull the macOS toolchain.

Bug:  742527 
Change-Id: Ic49785078f0e15c06f44f61a0daab54dd552ec9e
Reviewed-on: https://chromium-review.googlesource.com/598776
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Justin Cohen <justincohen@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491498}
[modify] https://crrev.com/a211039cca8b7afe0edc753d051772a5ec9d63d7/build/mac_toolchain.py

FWIW: I'm running into this problem (or one very much like it :-}) @ p491708 (i.e. after the above commit).  See  issue 752092  (which I think I'm going to close as a dup of this).  Removing target = ['ios'] from my .gclient fixed the problem, but I'd love a longer-term fix if possible.
 Issue 752092  has been merged into this issue.
rdsmith, are you synced up? Past 491498 this is supposed to just work.
@thakis: See c#17.  I'm seeing it at p491708, which I think is > 491498 :-}.
rdsmith: Are you on a corp image? Do you have a directory at '/Library/GoogleCorpSupport/'?
@erikchen: Yes, and yes.

Minor mistake that I don't think is relevant: I'm at 491609, not 491708.

At Erik's offline request, I re-added the target_os line and re-ran gclient sync, and I don't have the problem anymore.  The results of cat ./build/mac_files/toolchain_build_revision were 8E2002-3 both before and after the change.  So I'm not an issue anymore.
Labels: M-63

Comment 26 by h...@chromium.org, Aug 30 2017

I just ran into this.

I had
target_os = ['ios']
in my .gclient file, which apparently prevented gclient sync from downloading the new sdk.

Removing that line made it work.
A few questions and comments:

thakis@ Mac puts it's toolchain in build/mac_files, ios put it's toolchain build/ios_files -- so they shouldn't clobber one another. Is that not what you are seeing?

hans@ With erikchen@'s change I'm curious why the mac toolchain wasn't pulled.  Is _IsCorpMachine is False for you?

If `target_os = ['ios']` means mac + ios, does that mean all the ios bots and ios developers should also be pulling build/mac_files/Xcode?  iOS builds don't need this, but it doesn't roll that often.  It means about 3 minutes of time per roll and an extra 2.1GB on the bots and dev machines.  

But I'm not really sure how users are still seeing this unless _IsCorpMachine is False.


Project Member

Comment 29 by bugdroid1@chromium.org, Aug 31 2017

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

commit 9d09a66e5742982e0d2a8dbdee3726ddd8abbea1
Author: Justin Cohen <justincohen@google.com>
Date: Thu Aug 31 22:05:36 2017

Tweak logic in mac_toolchain.py

 - Download mac_toolchain always.
 - Download ios_toolchain if set in target_os and FORCE_MAC_TOOLCHAIN

This means all bots and developers will always have mac_toolchain,
which will fix the problem where sometimes dev set target_os=['ios']
and stop getting the mac toolchain.  iOS-only dev and bots will also get
mac_toolchain rolls, but this roll doesn't happen often, and only
adds a few minutes and few hundred MB occasionally.


Bug:  742527 
Change-Id: I6387c7b886cda96d9555591a750e21da94986228
Reviewed-on: https://chromium-review.googlesource.com/646449
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499033}
[modify] https://crrev.com/9d09a66e5742982e0d2a8dbdee3726ddd8abbea1/build/mac_toolchain.py

Status: Fixed (was: Assigned)
Hopefully now fixed!

Sign in to add a comment