CROS_WORKON_SUBDIRS_BLACKLIST never really worked for non cros-workon'ed packages. |
|||
Issue description... I think. In particular, the autotest-server ebuild (http://shortn/_5QP8zmQN0X) and other autotest related ebuilds use it to try to block some subdirectores from being installed. But that fails (https://bugs.chromium.org/p/chromium/issues/detail?id=392247) That bug says fixed but it is not (dshi@ fixed a different bug where autotest and autotest-server had collision) Doing this will cause errors: $ cros_workon --board guado_moblab stop chromeos-base/autotest-server $ emerge-guado_moblab chromeos-base/autotest-server # <OK> $ cros_workon --board guado_moblab start chromeos-base/autotest-server $ emerge-guado_moblab chromeos-base/autotest-server ## <ERROR> basically /build/guado_moblab/autotest/logs is installed by both chromeos-base/autotest-server-0.0.1-rXXXX.ebuild and chromeos-base/autotest-server-9999.ebuild file /build/guado_moblab/autotest/logs tells us that it is an actual directory instead of being a symlink (as the autotest-server ebuild wants it to be). Running the emerge with --debug tells us that the subfolder "logs" is being copied despite being in CROS_WORKON_SUBDIRS_BLACKLIST ---------------------- This was a cros-workon.eclass option introduced in https://chromium-review.googlesource.com/#/c/26108/ It is supposed to block source subdirectories from being copied into the WORKDIR _before_ a build. The catch is that it is *not guaranteed to exclude the said files* In particular, this will only remove the said sub-directories if local_copy is used to copy files over from a temporary git checkout of the repo (http://shortn/_qnICZbOJqm). OTOH, like in the normal developer flow, you have a local copy of the git project already in your chromiumos checkout, local_copy is never called. Instead we use git to do a local clone / symlink from the clone. In that case, the blacklisted subdirs end up in the build directory. Now (1) One can say that CROS_WORKON_SUBDIRS_BLACKLIST is a performance optimization, and should not be used to try to block files from getting installed (do something in src_install for that) (2) Or, one can try to make the behaviour consistent (and supported) for all paths through cros-workon_src_unpack. I'm leaning towards (1).
,
Dec 22 2016
I misunderstood the code a bit. This was always broken iff the package is cros_workon stop'ed. Since we do the git copy operation for cros_workon stopped packages only. Now I think (2) is possible and preferred. When we use git to clone the sources (either from already checkout out tree or from remote), we can go back and clean the blacklisted folders to get the same behaviour.
,
Dec 22 2016
CLs up: Fixes for some cases: https://chromium-review.googlesource.com/#/c/422446/ Die for others: https://chromium-review.googlesource.com/#/c/423187/ Note that CROS_WORKON_SUBDIRS_TO_COPY has a similar problem in that behaviour is dependent on exactly where the sources get copied from, which may not be obvious to the user at all. The documentation for that option at least says clearly that it's a performance optimization that doesn't always work. I personally think we should nix this option. It creates opportunity for non-incremental builds to be different in different environments. I don't know how much performance impact how many developers will have if we remove that option.
,
Dec 23 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/45d1ca9d64ba5fcc40685f0d5d0b7913c2f86897 commit 45d1ca9d64ba5fcc40685f0d5d0b7913c2f86897 Author: Prathmesh Prabhu <pprabhu@chromium.org> Date: Thu Dec 22 01:15:57 2016 cros-workon: Add some incompatible options validation. There is no way to enforce the sub-directory blacklist if we never copy sources from the user's checkout. This is OK, since this is not the intended use case for the CROS_WORKON_SUBDIR_BLACKLIST option. Let's just validate the ebuilds to be sure. BUG= chromium:676503 TEST=pre-cq Change-Id: I6097c449a88977b81bf781a489b46856ce8be851 Reviewed-on: https://chromium-review.googlesource.com/423187 Commit-Ready: Prathmesh Prabhu <pprabhu@chromium.org> Tested-by: Prathmesh Prabhu <pprabhu@chromium.org> Reviewed-by: Prathmesh Prabhu <pprabhu@chromium.org> [modify] https://crrev.com/45d1ca9d64ba5fcc40685f0d5d0b7913c2f86897/eclass/cros-workon.eclass
,
Dec 27 2016
Looks like chromeos-base/autotest unittest have been broken for a while: https://luci-milo.appspot.com/buildbot/chromiumos.tryserver/pre_cq/12023# because they depend on a blacklisted path being available. No one caught it because no one was actually testing with the package cros_workon'ed. I repro'd that the package fails locally in the same way when cros_workon'ed.
,
Dec 28 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/04d261ce0e935856768f8b076f517835f72f8dd3 commit 04d261ce0e935856768f8b076f517835f72f8dd3 Author: Prathmesh Prabhu <pprabhu@chromium.org> Date: Tue Dec 27 19:32:22 2016 chromeos-base/autotest: Remove needed directories from blacklist. chromeos-base/autotest had some directories in CROS_WORKON_SUBDIRS_BLACKLIST that are actually needed for unittests. FixIt. BUG= chromium:676503 TEST=$ cros_workon start chromeos-base/autotest $ emerge chromeos-base/autotest Change-Id: I5a53b3c6f669094f50b6896e5d6a27f0dd10257a Reviewed-on: https://chromium-review.googlesource.com/424159 Commit-Ready: Prathmesh Prabhu <pprabhu@chromium.org> Tested-by: Prathmesh Prabhu <pprabhu@chromium.org> Reviewed-by: Prathmesh Prabhu <pprabhu@chromium.org> [modify] https://crrev.com/04d261ce0e935856768f8b076f517835f72f8dd3/chromeos-base/autotest/autotest-9999.ebuild
,
Jan 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/crosutils/+/a6ba28ef08aae28c191da1e8d2d05b0e5d54de37 commit a6ba28ef08aae28c191da1e8d2d05b0e5d54de37 Author: Prathmesh Prabhu <pprabhu@chromium.org> Date: Thu Dec 29 20:29:55 2016 update_hooks: Cleanup .../autotest/logs directory. CL:422446 results in the directory autotest/logs getting converted to a symlink (as was the original intention). An ebuild uprev is not allowed to do this, so add a chroot hook. BUG= chromium:676503 TEST=- merge chromeos-base/autotest-server before CL:422446 - Verify that /build/${BOARD}/autotest/logs is a directory. - Run chroot_upgrade_hooks. - Verify that /build/${BOARD}autotest/logs is a symlink. Change-Id: Ic1501d0bf02e01c5b91544cb331d9b3f18e99fca Reviewed-on: https://chromium-review.googlesource.com/424320 Reviewed-by: Prathmesh Prabhu <pprabhu@chromium.org> Reviewed-by: Mike Frysinger <vapier@chromium.org> Tested-by: Prathmesh Prabhu <pprabhu@chromium.org> [add] https://crrev.com/a6ba28ef08aae28c191da1e8d2d05b0e5d54de37/chroot_version_hooks.d/146_autotest_server_logs_dir_to_symlink
,
Jan 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/c1306259d19f8a6d84e15aaf31f58c04413c2e7b commit c1306259d19f8a6d84e15aaf31f58c04413c2e7b Author: Prathmesh Prabhu <pprabhu@chromium.org> Date: Thu Dec 22 00:58:48 2016 cros-workon: Respect CROS_WORKON_SUBDIRS_BLACKLIST for non local fetch. Before this CL, CROS_WORKON_SUBDIRS_BLACKLIST was only respected when fetch_method=local. In effect, this meant that the blacklist was respected for *-9999.ebuild but not for the non -9999 counterpart. This CL brings the support closer to complete -- we still don't (and can't) respect the option when we decide to just symlink the already checked-out source of the developer. BUG= chromium:676503 TEST=Following passes: $ cros_workon --board guado_moblab stop chromeos-base/autotest-server $ emerge-guado_moblab chromeos-base/autotest-server $ cros_workon --board guado_moblab start chromeos-base/autotest-server $ emerge-guado_moblab chromeos-base/autotest-server CQ-DEPEND=CL:424159 complete. Change-Id: I13f3ec74568c47139d9481678c47df8dfdfe0248 Reviewed-on: https://chromium-review.googlesource.com/422446 Reviewed-by: Prathmesh Prabhu <pprabhu@chromium.org> Tested-by: Prathmesh Prabhu <pprabhu@chromium.org> [modify] https://crrev.com/c1306259d19f8a6d84e15aaf31f58c04413c2e7b/eclass/cros-workon.eclass
,
Jan 3 2017
autotest-server has built since #8 landed, and guado_moblab is not up in flames. |
|||
►
Sign in to add a comment |
|||
Comment 1 by pprabhu@chromium.org
, Dec 22 2016