-I /mnt/host/source/src/platform2 breaks the CROS_WORKON_SUBTREE sandbox |
|||||
Issue descriptionWe add platform2 to the list of directories in the include search path here: https://chromium.googlesource.com/chromiumos/platform2/+/master/common-mk/common.gypi#90 GYP does this by using the -I flag. However, the -I flag adds the given directory to the _system_ include search directories. This means that when there is a header file that is present in both $SYSROOT/usr/include and /mnt/host/source/src/platform2 (like libpasswordprovider/password.h), it's completely random whether the compiler would use the header from the board's sysroot or from platform2. See https://logs.chromium.org/v/?s=chromeos%2Fbb%2Fchromeos%2Fpoppy-paladin%2F4643%2F%2B%2Frecipes%2Fsteps%2FBuildPackages%2F0%2Fstdout It seems that the compiler doesn't necessarily care about the order in which the paths are given on the command line (which makes sense, there's no reason for that to matter). This wasn't that big of a problem before but now that we've started enforcing a file sandbox, an unlucky run where the compiler puts platform2 before the board's sysroot result in a sandbox violation and a failed run of the package. AFAICT there's no reason for platform2 to ever be in the system include search directories. We should always include it with -iquote instead of -I. This will ensure that the compiler will never search that path for files included via angle brackets. However, I can't see a way to do this with GYP. It seems like anything in the "include_dirs" rule always uses the -I flag. Thoughts? Marking as P1 because this is resulting in flaky builds where the smbprovider package can sometimes fail to build the first time.
,
Apr 17 2018
Unrelated but is there a plan somewhere for the migration to GN? My understanding is that it's hyper focused for chrome's use case and doesn't really work well for things that don't fit into that use case. In particular I'd be interested in seeing the reasons for using GN over something like bazel.
,
Apr 17 2018
Re #1: Setting -iquote is possible in gyp, technically. platform2/common-mk/common.gypi L87 is the configuration adding platform2/ to the include path. You can specify -iquote flags by replacing it with cflags. As for gn, we should be able to do something similar. I don't have any concern on it, now. (off topic): Re #2: I still think GN is better option. I'm preparing the design doc, and will share it in a day or two.
,
Apr 17 2018
I took a quick look. Setting -iquote looks easy (as hidehiko@ mentioned), but updating existing #include needs some non-trivial work.
,
Apr 17 2018
Other possible approaches: - Avoid CROS_WORKON_OUTOFTREE_BUILD. - Enhance portage sandbox to hide files rather than denying access.
,
Apr 18 2018
I tried extending portage sandbox to hide files, but rewriting getdents(2) results seemed non-trivial. Now I'm wondering if we can make OUTOFTREE_BUILD and SUBTREE exclusive. If we consider SUBTREE on rsync, I guess copy cost will be not very large.
,
Apr 18 2018
-> nya since you have start the investigation.
,
Apr 20 2018
TL;DR -iquote is not a great option. I tried -iquote and adjusting all #include in platform2, and found -iquote does not work well in some cases. For example, libpasswordprovider/password_provider.h includes libpasswordprovider/libpasswordprovider_export.h. Which #include should we use for this, <> or ""? The answer does not exist because: - When it is included from libpasswordprovider/password_provider.cc, then it should include platform2/libpasswordprovider/libpasswordprovider_export.h, so we should use "". - When it is included from login_manager/session_manager_impl.cc, then it should include /usr/include/libpasswordprovider/libpasswordprovider_export.h, so we should use <>. So it seems like differentiating <> and "" is a bad idea.
,
Apr 20 2018
In that case, password.h should include it as:
#include "libpasswordprovider_export.h"
This will work no matter password.h itself gets included as long as both files are in the same directory. This is how every other library that includes multiple headers works. You can see for yourself by running:
find /build/eve/usr/include -name '*.h' -exec grep -H -e '#include.*".*"' {} \;
Some examples:
/build/eve/usr/include/nss/secdigt.h:#include "secitem.h"
/build/eve/usr/include/nss/ssl.h:#include "prtypes.h"
/build/eve/usr/include/nss/ssl.h:#include "prerror.h"
/build/eve/usr/include/nss/ssl.h:#include "prio.h"
/build/eve/usr/include/nss/ssl.h:#include "seccomon.h"
/build/eve/usr/include/nss/ssl.h:#include "cert.h"
/build/eve/usr/include/nss/ssl.h:#include "keyt.h"
/build/eve/usr/include/zbar/Window.h:#include "Image.h"
/build/eve/usr/include/zbar/Processor.h:#include "Exception.h"
/build/eve/usr/include/zbar/Processor.h:#include "Image.h"
/build/eve/usr/include/zbar/Image.h:#include "Symbol.h"
/build/eve/usr/include/zbar/Image.h:#include "Exception.h"
So I still think -iquote is the proper way to handle this. Headers are differentiated by <> and "" for a reason. libpasswordprovider is doing it wrong.
,
Apr 23 2018
Hmm, I'm not familiar at all about known best practices about #include, but I also see many #include <non-sys-header> in /usr/include. For example: /build/betty/usr/include/X11/Xresource.h:#include <X11/Xlib.h> /build/betty/usr/include/gtest/internal/gtest-param-util.h:#include "gtest/internal/gtest-port.h" /build/betty/usr/include/libltdl/lt_error.h:#include <libltdl/lt_system.h> /build/betty/usr/include/avahi-common/thread-watch.h:#include <avahi-common/watch.h> /build/betty/usr/include/alsa/asoundlib.h:#include <alsa/hwdep.h> Also I'm not sure of the rules you want to use in our code base, please let me clarify. For instance, platform2/cryptohome/cert/cert_provision_util.h includes platform2/cryptohome/cert_provision.h and cert_provision_util.h is not installed to sysroot today. In this case how should we #include? I guess the answer is either of the following two: 1. #include "cryptohome/cert_provision.h" 2. #include "../cert_provision.h" If you think (1) is right: then what if we start to install cert_provision_util.h to sysroot? Do we need to rewrite #include according to whether we install those headers in sysroot? If you think (2) is right: then I think we don't need -iquote at all, relative paths are always searched.
,
Apr 23 2018
,
Apr 23 2018
What I would like to see is all that all public headers go into a single directory and refer to other public headers using paths relative their current location. So we might have something like:
platform2/some_project/public/foo.h
platform2/some_project/public/bar.h
platform2/some_project/public/nested/baz.h
foo.h would include bar.h as
#include "bar.h"
and baz.h would be
#include "nested/baz.h"
If baz.h wants to include foo.h, it would do
#include "../foo.h"
However, I would strongly recommend against having nested public headers depend on headers higher in the tree. All these headers would get installed in the ebuild as:
insinto /usr/include/some_project
doins -r some_project/public/*
The other rule I would like to enforce is that no public header should depend on a non-public header. So in your example, if we start to install cert_provision_util.h to the sysroot then it and all the headers it depends on should go into the public header directory and the includes should be re-written to match public header rules.
Making an existing header file public is not something that should be done lightly and this will make people think about what they are actually exposing to their clients.
-iquote is still needed because we still want people to include non-public headers using the full path relative to the platform2 directory.
,
Apr 25 2018
Thanks for clarification, chirantan. While I totally agree splitting headers into public/private is clean, it's fairly large amount of work even if we consider platform2 only. Let's go back to the first problem: why is it "random" whether $SYSROOT/usr/include or /mnt/host/source/src/platform2 is preferred? GCC manual says: > The lookup order is as follows: > 1. For the quote form of the include directive, the directory of the current file is searched first. > 2. For the quote form of the include directive, the directories specified by -iquote options are searched in left-to-right order, as they appear on the command line. > 3. Directories specified with -I options are scanned in left-to-right order. > 4. Directories specified with -isystem options are scanned in left-to-right order. > 5. Standard system directories are scanned. > 6. Directories specified with -idirafter options are scanned in left-to-right order. https://gcc.gnu.org/onlinedocs/gcc/Directory-Options.html These is no ambiguity in the order defined here. What makes it random in Chrome OS builds?
,
Apr 25 2018
The rules as defined will break the CROS_WORKON_SUBTREE sandbox 100% of the time. If there is both "platform2/foo/bar.h" and "/build/$BOARD/usr/include/foo/bar.h", then according to the gcc rules it will always look in the platform2 directory first since directories specified with -I (rule 3) are searched before standard system directories (rule 5). I have no idea why this is random in chrome os builds. It sounds like it should be failing every time.
,
Jun 11 2018
The following revision refers to this bug: https://chrome-internal.googlesource.com/chromeos/ap/+/f2f38acc0a3c5af555723ce83e4be60aa591cf2a commit f2f38acc0a3c5af555723ce83e4be60aa591cf2a Author: Mike Frysinger <vapier@chromium.org> Date: Mon Jun 11 05:13:59 2018
,
Jun 11 2018
The following revision refers to this bug: https://chrome-internal.googlesource.com/chromeos/thermald/+/3b8c66cce9c26903c0de002be3380edcc64b0d55 commit 3b8c66cce9c26903c0de002be3380edcc64b0d55 Author: Mike Frysinger <vapier@chromium.org> Date: Mon Jun 11 08:02:04 2018
,
Jun 11 2018
The following revision refers to this bug: https://chrome-internal.googlesource.com/chromeos/platform/arc-oemcrypto/+/420c1b1e9c98cb2c370029dde26630cff2e10d7c commit 420c1b1e9c98cb2c370029dde26630cff2e10d7c Author: Mike Frysinger <vapier@chromium.org> Date: Mon Jun 11 10:34:54 2018
,
Jun 11 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/cfm-device-monitor/+/244ca971853491aad1f045b478f9853d4e2d97d6 commit 244ca971853491aad1f045b478f9853d4e2d97d6 Author: Mike Frysinger <vapier@chromium.org> Date: Mon Jun 11 20:44:36 2018 add include path for this repo We're removing the include path to src/platform/ from the platform2 gypi, so these projects need to set it themselves. BUG=chromium:833675 TEST=precq passes Change-Id: I3af3cda6a9be520b0125b89678c9fa160bc88f9f Reviewed-on: https://chromium-review.googlesource.com/1082380 Commit-Ready: Mike Frysinger <vapier@chromium.org> Tested-by: Mike Frysinger <vapier@chromium.org> Reviewed-by: Shuhei Takahashi <nya@chromium.org> [modify] https://crrev.com/244ca971853491aad1f045b478f9853d4e2d97d6/cfm-device-monitor.gyp
,
Jun 18 2018
,
Aug 20
The following revision refers to this bug: https://chrome-internal.googlesource.com/chromeos/platform/gdisp/+/58ca435d262a2b2f6b0b7251965a684fcce90512 commit 58ca435d262a2b2f6b0b7251965a684fcce90512 Author: Mike Frysinger <vapier@chromium.org> Date: Mon Aug 20 10:27:38 2018
,
Aug 21
The following revision refers to this bug: https://chrome-internal.googlesource.com/chromeos/platform/actions/+/33e183d0cd619f0064b813f0c3bc3d0f184da741 commit 33e183d0cd619f0064b813f0c3bc3d0f184da741 Author: Mike Frysinger <vapier@chromium.org> Date: Tue Aug 21 00:12:14 2018
,
Aug 21
The following revision refers to this bug: https://chrome-internal.googlesource.com/chromeos/ap-daemons/+/ab66652a80320cd8978d38558d7a78e1df3f4c31 commit ab66652a80320cd8978d38558d7a78e1df3f4c31 Author: Mike Frysinger <vapier@chromium.org> Date: Tue Aug 21 00:12:14 2018
,
Aug 21
The following revision refers to this bug: https://chrome-internal.googlesource.com/chromeos/ap/security/+/6344da252efaeae5f4427aabf4c14c8ee484790a commit 6344da252efaeae5f4427aabf4c14c8ee484790a Author: Mike Frysinger <vapier@chromium.org> Date: Tue Aug 21 00:12:20 2018
,
Aug 21
The following revision refers to this bug: https://chrome-internal.googlesource.com/chromeos/ap/wireless/+/3ed6243fe037dbba07ceb95bf2a7eadc88c7e985 commit 3ed6243fe037dbba07ceb95bf2a7eadc88c7e985 Author: Mike Frysinger <vapier@chromium.org> Date: Tue Aug 21 04:42:59 2018
,
Aug 23
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/30d168dba734761e1bed7a1a659f678d86e15d80 commit 30d168dba734761e1bed7a1a659f678d86e15d80 Author: Mike Frysinger <vapier@chromium.org> Date: Thu Aug 23 20:20:43 2018 common-mk: stop adding include path to src/platform/ All our projects should be isolated now, so if they're still relying on this implicit -I path to src/platform/, we want to fix them rather than keep letting them be broken. BUG=chromium:833675 TEST=precq passes CQ-DEPEND=CL:*634234,CL:*634233,CL:*634232,CL:1082380 Change-Id: I690cc7e185da6a7966a25107b4b495e1bec641ae Reviewed-on: https://chromium-review.googlesource.com/1081449 Commit-Ready: Mike Frysinger <vapier@chromium.org> Tested-by: Mike Frysinger <vapier@chromium.org> Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org> Reviewed-by: Miao-chen Chou <mcchou@chromium.org> [modify] https://crrev.com/30d168dba734761e1bed7a1a659f678d86e15d80/common-mk/common.gypi
,
Aug 23
so the common code no longer adds a -I flag to the root, but i had to basically push that hack down to a bunch of random packages that actually needed it. i'm not sure that this really helps with this bug or just makes it more controlled. you want to take another look ? |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by nya@chromium.org
, Apr 17 2018Components: Infra>Client>ChromeOS>Build