Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1181213003:
Pass 'src' explicitly to non-nacl (naclports builds). (Closed)
DescriptionPass 'src' explicitly to non-nacl (naclports builds).
Cleanup leftovers from gyp.
BUG=None
TEST=None
R=sbc@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=295658
Patch Set 1 #Patch Set 2 : fix #
Total comments: 7
Patch Set 3 : fix #Messages
Total messages: 17 (5 generated)
lgtm, but we should probably get someone from infra to check this out. https://codereview.chromium.org/1181213003/diff/20001/masters/master.tryserve... File masters/master.tryserver.nacl/master.cfg (right): https://codereview.chromium.org/1181213003/diff/20001/masters/master.tryserve... masters/master.tryserver.nacl/master.cfg:636: # Set "root" build property to "native_client" in all nacl-* builders. Update the comment. https://codereview.chromium.org/1181213003/diff/20001/masters/master.tryserve... masters/master.tryserver.nacl/master.cfg:641: b.setdefault('properties', {})['root'] = 'src' How does this work on other waterfalls?
https://codereview.chromium.org/1181213003/diff/20001/masters/master.tryserve... File masters/master.tryserver.nacl/master.cfg (right): https://codereview.chromium.org/1181213003/diff/20001/masters/master.tryserve... masters/master.tryserver.nacl/master.cfg:636: # Set "root" build property to "native_client" in all nacl-* builders. On 2015/06/12 16:55:46, Sam Clegg wrote: > Update the comment. Done. https://codereview.chromium.org/1181213003/diff/20001/masters/master.tryserve... masters/master.tryserver.nacl/master.cfg:641: b.setdefault('properties', {})['root'] = 'src' On 2015/06/12 16:55:46, Sam Clegg wrote: > How does this work on other waterfalls? Unclear, my impression is that for many others a recipes has been written to replace this entire set of plumbing. A new team member was assigned to do this for our waterfalls, but it hasn't happened yet.
nodir@chromium.org changed reviewers: + nodir@chromium.org
https://codereview.chromium.org/1181213003/diff/20001/masters/master.tryserve... File masters/master.tryserver.nacl/master.cfg (left): https://codereview.chromium.org/1181213003/diff/20001/masters/master.tryserve... masters/master.tryserver.nacl/master.cfg:644: Can't review this part https://codereview.chromium.org/1181213003/diff/20001/masters/master.tryserve... File masters/master.tryserver.nacl/master.cfg (right): https://codereview.chromium.org/1181213003/diff/20001/masters/master.tryserve... masters/master.tryserver.nacl/master.cfg:641: b.setdefault('properties', {})['root'] = 'src' this part lgtm https://codereview.chromium.org/1181213003/diff/20001/masters/master.tryserve... masters/master.tryserver.nacl/master.cfg:641: b.setdefault('properties', {})['root'] = 'src' On 2015/06/12 17:05:08, bradn wrote: > On 2015/06/12 16:55:46, Sam Clegg wrote: > > How does this work on other waterfalls? > > Unclear, my impression is that for many others a recipes has been written to > replace this entire set of plumbing. A new team member was assigned to do this > for our waterfalls, but it hasn't happened yet. Other waterfalls use recipes. Recipes simply do not expect the "root" property to be set. NaCl needs this because it does not use recipes
The CQ bit was checked by bradnelson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sbc@chromium.org Link to the patchset: https://codereview.chromium.org/1181213003/#ps40001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1181213003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=295658
Message was sent while issue was closed.
On 2015/06/12 17:11:12, nodir wrote: > https://codereview.chromium.org/1181213003/diff/20001/masters/master.tryserve... > File masters/master.tryserver.nacl/master.cfg (left): > > https://codereview.chromium.org/1181213003/diff/20001/masters/master.tryserve... > masters/master.tryserver.nacl/master.cfg:644: > Can't review this part > > https://codereview.chromium.org/1181213003/diff/20001/masters/master.tryserve... > File masters/master.tryserver.nacl/master.cfg (right): > > https://codereview.chromium.org/1181213003/diff/20001/masters/master.tryserve... > masters/master.tryserver.nacl/master.cfg:641: b.setdefault('properties', > {})['root'] = 'src' > this part lgtm > > https://codereview.chromium.org/1181213003/diff/20001/masters/master.tryserve... > masters/master.tryserver.nacl/master.cfg:641: b.setdefault('properties', > {})['root'] = 'src' > On 2015/06/12 17:05:08, bradn wrote: > > On 2015/06/12 16:55:46, Sam Clegg wrote: > > > How does this work on other waterfalls? > > > > Unclear, my impression is that for many others a recipes has been written to > > replace this entire set of plumbing. A new team member was assigned to do this > > for our waterfalls, but it hasn't happened yet. > > Other waterfalls use recipes. Recipes simply do not expect the "root" property > to be set. NaCl needs this because it does not use recipes Thanks. Can either of you explain the strange output from the apply_issue command in the bot_update step: http://build.chromium.org/p/tryserver.nacl/builders/naclports-linux-newlib-4/... ===Running apply_issue --root_dir --issue 1162113008 --server https://codereview.chromium.org --force --ignore_deps --no-auth --patchset 60001 (retry #1)=== Connecting to https://codereview.chromium.org Downloading the patch. ports/glibc-compat/include/sys/time.h ports/glibc-compat/include/sys/types.h N ports/gzip/build.sh N ports/gzip/nacl.patch N ports/gzip/pkg_info Applying the patch. ports/glibc-compat/include/sys/time.h Created missing directory ports/glibc-compat/include/sys. ports/glibc-compat/include/sys/types.h ports/gzip/build.sh Created missing directory ports/gzip. ports/gzip/nacl.patch ports/gzip/pkg_info Found unpatched files: [u'ports/glibc-compat/include/sys/time.h', u'ports/glibc-compat/include/sys/types.h', u'ports/gzip/build.sh', u'ports/gzip/nacl.patch', u'ports/gzip/pkg_info'] ===Succeeded in 0.0 mins=== Based on the out from the this it looks like it must be running checkout.py:632(GitCheckout.apply_issue) since this is the only version of apply_issue that prints "Found unpatched files". given the thay apply_issue was bring run in the wrong directory, i would have expected the 'git apply' command on line 682 to have failed, at least for the two files in the patch that were not new files. TBH I would have expected it to fail completely since git would be been run from the top level directory, which is not a git checkout. Very strange.
Message was sent while issue was closed.
On 2015/06/12 17:22:40, Sam Clegg wrote: > On 2015/06/12 17:11:12, nodir wrote: > > > https://codereview.chromium.org/1181213003/diff/20001/masters/master.tryserve... > > File masters/master.tryserver.nacl/master.cfg (left): > > > > > https://codereview.chromium.org/1181213003/diff/20001/masters/master.tryserve... > > masters/master.tryserver.nacl/master.cfg:644: > > Can't review this part > > > > > https://codereview.chromium.org/1181213003/diff/20001/masters/master.tryserve... > > File masters/master.tryserver.nacl/master.cfg (right): > > > > > https://codereview.chromium.org/1181213003/diff/20001/masters/master.tryserve... > > masters/master.tryserver.nacl/master.cfg:641: b.setdefault('properties', > > {})['root'] = 'src' > > this part lgtm > > > > > https://codereview.chromium.org/1181213003/diff/20001/masters/master.tryserve... > > masters/master.tryserver.nacl/master.cfg:641: b.setdefault('properties', > > {})['root'] = 'src' > > On 2015/06/12 17:05:08, bradn wrote: > > > On 2015/06/12 16:55:46, Sam Clegg wrote: > > > > How does this work on other waterfalls? > > > > > > Unclear, my impression is that for many others a recipes has been written to > > > replace this entire set of plumbing. A new team member was assigned to do > this > > > for our waterfalls, but it hasn't happened yet. > > > > Other waterfalls use recipes. Recipes simply do not expect the "root" property > > to be set. NaCl needs this because it does not use recipes > > Thanks. > > Can either of you explain the strange output from the apply_issue command in the > bot_update step: > > http://build.chromium.org/p/tryserver.nacl/builders/naclports-linux-newlib-4/... > ===Running apply_issue --root_dir --issue 1162113008 --server > https://codereview.chromium.org --force --ignore_deps --no-auth --patchset 60001 > (retry #1)=== > Connecting to https://codereview.chromium.org > Downloading the patch. > ports/glibc-compat/include/sys/time.h > ports/glibc-compat/include/sys/types.h > N ports/gzip/build.sh > N ports/gzip/nacl.patch > N ports/gzip/pkg_info > > Applying the patch. > ports/glibc-compat/include/sys/time.h > Created missing directory ports/glibc-compat/include/sys. > > ports/glibc-compat/include/sys/types.h > > ports/gzip/build.sh > Created missing directory ports/gzip. > > ports/gzip/nacl.patch > > ports/gzip/pkg_info > > Found unpatched files: [u'ports/glibc-compat/include/sys/time.h', > u'ports/glibc-compat/include/sys/types.h', u'ports/gzip/build.sh', > u'ports/gzip/nacl.patch', u'ports/gzip/pkg_info'] > ===Succeeded in 0.0 mins=== > > > Based on the out from the this it looks like it must be running > checkout.py:632(GitCheckout.apply_issue) since this is the only version of > apply_issue that prints "Found unpatched files". > > given the thay apply_issue was bring run in the wrong directory, i would have > expected the 'git apply' command on line 682 to have failed, at least for the > two files in the patch that were not new files. TBH I would have expected it > to fail completely since git would be been run from the top level directory, > which is not a git checkout. > > Very strange. When I run the command locally I get: $ apply_issue --root_dir "" --issue 1162113008 --server https://codereview.chromium.org --force --ignore_deps --no-auth --patchset 60001 Connecting to https://codereview.chromium.org Downloading the patch. ports/glibc-compat/include/sys/time.h ports/glibc-compat/include/sys/types.h N ports/gzip/build.sh N ports/gzip/nacl.patch N ports/gzip/pkg_info Applying the patch. Failed to apply patch for ports/glibc-compat/include/sys/time.h: While running patch -u --binary -p1 --verbose; Hmm... Looks like a unified diff to me... can't find file to patch at input line 6 Perhaps you used the wrong -p or --strip option? The text leading up to this was: -------------------------- |Index: ports/glibc-compat/include/sys/time.h |diff --git a/ports/glibc-compat/include/sys/time.h b/ports/glibc-compat/include/sys/time.h |index 94cce6aaa753aba3a118ab55772d6299b88bb07e..c28bb14add839b34463899d39f44ed77a1d0b695 100644 |--- a/ports/glibc-compat/include/sys/time.h |+++ b/ports/glibc-compat/include/sys/time.h -------------------------- File to patch: Skip this patch? [y] Skipping patch. Hunk #1 ignored at 1. 1 out of 1 hunk ignored done Patch: ports/glibc-compat/include/sys/time.h Index: ports/glibc-compat/include/sys/time.h diff --git a/ports/glibc-compat/include/sys/time.h b/ports/glibc-compat/include/sys/time.h index 94cce6aaa753aba3a118ab55772d6299b88bb07e..c28bb14add839b34463899d39f44ed77a1d0b695 100644 --- a/ports/glibc-compat/include/sys/time.h +++ b/ports/glibc-compat/include/sys/time.h @@ -1,5 +1,25 @@ +/* + * Copyright (c) 2014 The Native Client Authors. All rights reserved. + * Use of this source code is governed by a BSD-style license that can be + * found in the LICENSE file. + */ #ifndef GLIBCEMU_SYS_TIME_H #define GLIBCEMU_SYS_TIME_H 1 + +/* gnulib hack (it builds its own headers sometimes) + Include guard to prevent gnulib from defining conflicting types + bug at: + https://code.google.com/p/nativeclient/issues/detail?id=4198 +*/ +#if defined(__native_client__) +#include <stdint.h> +typedef int64_t _off_t; +typedef int64_t __dev_t; +typedef uint32_t __uid_t; +typedef uint32_t __gid_t; +typedef int32_t _ssize_t; +#endif + #include_next <sys/time.h> /* Convenience macros for operations on timevals. CWD=/home/sbc/dev/naclports Checkout path=/home/sbc/dev/naclports
Message was sent while issue was closed.
@Sam: root dir is not set, it is taken from the root property. The master must be restarted for these changes to take effect
Message was sent while issue was closed.
On 2015/06/12 17:36:32, nodir wrote: > @Sam: root dir is not set, it is taken from the root property. The master must > be restarted for these changes to take effect I can see the empty root_dir, I'm just wondering why the apply_patch on the bot didn't fail as expected.
Message was sent while issue was closed.
nodir@chromium.org changed reviewers: + nodir@google.com
Message was sent while issue was closed.
+hinoka (bot_update author) for Sam's question
Message was sent while issue was closed.
hinoka@chromium.org changed reviewers: + hinoka@chromium.org
Message was sent while issue was closed.
good question, I don't know. It could've ended up trying to patch the slave's build checkout. |
