My preference (but can be convinced otherwise) would be to specify the flags in the isolate recipe consistently, instead of hard-coding in the tool like it's currently attempted.
Local testing suggests that DEFAULT_BLACKLIST is working properly.
"""
erikchen@erikchen-macpro3 ~/projects/chromium/src (test54)$ cat out/gn/asdf.isolate
{'variables': {'command': ['dummy'],
'files': ['../../third_party/pywebsocket/src/mod_pywebsocket/']}}
erikchen@erikchen-macpro3 ~/projects/chromium/src (test54)$ ls third_party/pywebsocket/src/mod_pywebsocket/ | grep pyc | wc -l
15
python tools/swarming_client/isolate.py archive -i out/gn/asdf.isolate -s out/gn/asdf2.isolated --isolate-server https://isolateserver.appspot.
com
erikchen@erikchen-macpro3 ~/projects/chromium/src (test54)$ cat out/gn/asdf2.isolated | grep pyc
<nothing>
"""
Need to figure out why this isn't working for webkit_layout_test. This suggests that a client is explicitly passing in "--blacklist" and not using the default.
Spoke with tandrii@, next steps are:
"""
1. Land CL in luci-go.
2. Roll luci-go into infra/infra.
3. isolate tool is built by CI of infra/infra here
https://ci.chromium.org/p/infra-internal/g/infra-packagers/console
These builders only upload CIPD packages of isolate tool.
4. chromium/src and v8/v8 repos pin/deploy isolate tool in two ways:
a) legacy hash through googlestorage.
b) as CIPD package.
TODAY, both a) and b) must work for some reason (https://crbug.com/851596)
BUT step 3 only creates CIPD package. So, either you manually upload 3 binaries to some GS bucket, OR resolve https://crbug.com/851596.
5. Finally, roll isolate tool into chromium/src and v8/v8.
6. Land change to recipe which starts using --blacklist flag.
...
now, if you get buy in from jbudorick@, then you could "cheat" and do what you thought makes sense all along: add a step to isolate recipe itself to install the required CIPD package (regardless of 4. above)
path = api.cipd.install(...)
api.step([path + 'isolatego', batcharchive, ....)
IMO, this is much better way. However, there is a probably a reason why https://crbug.com/851596 is filed instead of doing the "cheat" way.
and that's why i recommend consulting jbudorick@
"""
jbudorick: See c#11. Any reason why we can't do what tandrii@ suggested and "add a step to isolate recipe itself to install the required CIPD package (regardless of 4. above) ".
That ... doesn't seem terribly cheaty to me. The only reason I can think of to use isolate from the chromium checkout is that we want it there regardless (s.t. devs can isolate things, even if they don't frequently do so) & it'd be great to not manage multiple pins.
All of that said, I believe maruel is close to eliminating 4a, which may make this mostly irrelevant.
#14,16: as of late yesterday, it should be safe from an ios internal perspective. not sure about v8, might be worth checking with +sergiyb or +machenbach for that.
AFAICT from [1], new binaries would need to be uploaded to gs://chromium-luci/v8/tools/luci-go/win64, yet I can only see files named after some hash in the root dir of that bucket. How does it even work now? Or does it use that 'v8/tools/...' path to get hash somewhere?
I've also tried switching V8 to CIPD, but something didn't work there: https://chromium-review.googlesource.com/c/v8/v8/+/1144923. I don't remember the details, but somehow I got stuck investigating those failures. Let me try to rebase that CL and see if it works now...
[1]: https://cs.chromium.org/chromium/src/v8/DEPS?l=183&rcl=33b726db1e2a5b7de8ddca88e165596c34d08a0d
Yes, that's essentially identical to the CL that I referenced in #18. When you are saying that "v8/tools/luci-go" is already mapped - do you mean that it's used for a regular repository dep? Didn't Chromium have to do the same change? Why doesn't it work?
Chromium's DEPS doesn't map anything in "src/tools/luci-go", since it's already mapped. That said we can probably just remove this map, it is not needed.
Ah, right. You meant that Chromium never had a dep for src/tools/luci-go because it was part of the repository itself. What do you mean that it is not needed... my understanding is that isolate binaries will then not be checked out and bots will start to fail, won't they?
Wait... in your CL, you are just adding a new dep without removing an old one. I've actually tried removing the old one in my CL, but that still didn't work.
Why not do everything in one CL?
Are the CLs 1 and 2 meant to both touch every bot? Note that due to having bots in a pool we might have some bots that go from 0 to 2 at once.
Making CL 1 that way seems not possible. I think gclient first syncs without patch, checking out the sha1 files. Then attempts to apply the patch, based on which those are local files now. Patch doesn't apply claiming the files'd get overwritten.
I've tried rebasing my CL from #18 and Michale's CL from #33:
- https://crrev.com/c/1144923 (replacing normal dep with CIPD dep in a single CL)
- https://ci.chromium.org/p/v8/builders/luci.v8.try/v8_linux64_rel_ng/b8931886382339502208
- still deleting luci-go after checking out the CIPD package, thus deleting isolate binaries
- despite that it SUCCEEDS isolating compiled binaries and triggered trybots are green as well
- https://crrev.com/c/1219386 (removing dep and moving SHA1 files to V8 repo - step 1 from #31)
- https://ci.chromium.org/p/v8/builders/luci.v8.try/v8_presubmit/b8931887457988355952
- error: The following untracked working tree files would be overwritten by merge
- seems like we can't remove dependency and add binaries to the same dir in a single CL
Perhaps we can try landing https://crrev.com/c/1144923 as is, but I'd like to know why it works now. Did someone update isolate step logic to not depend on those binaries? Or could it be that these binaries are now deployed by swarming automatically?
Hm. I am not sure if it's working as intended. The stdout of the bot_update step still lists
________ deleting 'v8/tools/luci-go' in '/b/swarming/w/ir/cache/builder'
after
v8/tools/luci-go:infra/tools/luci/isolate/${platform} (Elapsed: 0:00:00)
----------------------------------------
[0:00:07] Started.
[0:00:07] Finished.
----------------------------------------
and in the end fails to detect luci-go CIPD dep's revision:
===Running /b/swarming/w/ir/cache/vpython/998566/bin/python -u /b/swarming/w/ir/kitchen-checkout/depot_tools/gclient.py revinfo -a ===
In directory: /b/swarming/w/ir/cache/builder
...
v8/tools/luci-go:infra/tools/luci/isolate/${platform}: None
...
===Succeeded in 0.0 mins of /b/swarming/w/ir/cache/vpython/998566/bin/python -u /b/swarming/w/ir/kitchen-checkout/depot_tools/gclient.py revinfo -a ===
WARNING: Couldn't match revinfo line:
v8/tools/luci-go:infra/tools/luci/isolate/${platform}: None
Eward, can you please comment if this is expected?
The output is not clear at all for CIPD deps, but I think things are working.
- revinfo doesn't work for CIPD dependencies, so it will always display None.
- The step that syncs CIPD dependencies runs after the git dependency is deleted, and it doesn't display anything afaik.
- The step that prints "v8/tools/luci-go:infra/tools/luci/isolate/${platform} (Elapsed: 0:00:00)" doesn't do anything.
I'll make some CLs to make this clearer, but it'll take a while.
Thank you for clarifications. Fixing output is non-blocking for this issue, therefore non urgent. I'll wait for machenbach@'s LGTM on my CL and land it, which should unblock removing the old way to fetch isolate.
On erikchen's reland CL (https://chromium-review.googlesource.com/c/chromium/tools/build/+/1300234) he's still getting:
"""
/b/swarming/w/ir/cache/builder/src/tools/swarming_client batcharchive ...
flag provided but not defined: -blacklist
"""
I suppose that means that while luci-go is now deployed everywhere via cipd, not everything uses the "latest" luci-go cipd package that has erikchen's change https://chromium-review.googlesource.com/1176121 from 1.5 months ago? What needs to happen to make sure everything has a version of luci-go that's newer than 1..5 months old?
Where does "/b/swarming/w/ir/cache/builder/src/tools/swarming_client" come from? That's not a binary that LUCI provides, maybe the chromium recipe or chromium/src have a reference to this binary?
Maybe; I think maruel has kept tabs on all the ways the various swarming/isolate clients are distributed. I'm still not clear on the distinction between the go client and the python client (i.e. why do we have both?)
Not completely. But I think Joshua's work on issue 894048 will help unblock this *once for all*. I'm ambivalent about marking it as a blocker though. Erik please reach out to Josh and decide what's best. Also keep Stephen in the loop since he worked in this area.
Comment 1 by erikc...@chromium.org
, Aug 8Status: Assigned (was: Untriaged)