Add func KATI_visibility_prefix #275

mrziwang · 2024-04-17T23:29:09Z

syntax: $(KATI_visibility_prefix var, prefix)

Add a func KATI_visibility_prefix that takes a variable name and a list of strings, set this variable's visibility to these strings. Each string represents the relative path from the root, and is considered as prefix.

e.g. $(KATI_visibility_prefix VAR, vendor/ device/b baz.mk) --> VAR is visible to "vendor/foo.mk", "device/bar.mk", "device/baz.mk", "baz.mk", but not visible to "bar.mk", "vendor.mk" or "vendor/baz.mk".

If variable visibility is set more than once, and with a different list of strings, an error will occur.

When a variable is being referenced, if this variable has visibility prefix set, check if the current referencing file matches the visibility prefix. Throw an error if not.
In $(KATI_visibility_prefix FOO, prefix):
If FOO is not defined, create a variable FOO with empty value.
The prefix can also be reference to variable. If so, this function will expand the reference and set the visibility_prefix.

lamontj

One minor nit.

lamontj · 2024-04-17T23:43:43Z

src/var.cc

+  std::string referencingFile(loc.filename);
+  bool valid = false;
+  for (const std::string& prefix : prefixes) {
+    if (referencingFile.find(prefix) == 0) {


It would seem that the preferred syntax here is: if HasPrefix(referencingFile, prefix) {

+1, and if you use HasPrefix you can use loc.filename directly instead of creating a copy of it in referencingFile.

Also from our discussion, this should only check path prefixes instead of string prefixes (i.e. ven is not a prefix of vendor/)

This is how visibility works in soong/bazel, see https://cs.android.com/android/platform/superproject/main/+/main:build/soong/android/visibility.go;l=174;drc=9a24d909369ae20566c85dfbfe8b14054ed6dd5d

re: prefix matching -- sounds good to me.

Colecf · 2024-04-17T23:51:35Z

src/var.h

@@ -73,6 +73,10 @@ class Var : public Evaluable {
  bool SelfReferential() const { return self_referential_; }
  void SetSelfReferential() { self_referential_ = true; }

+  std::vector<std::string> VisibilityPrefix() const { return visibility_prefix_; }


Return const std::vector<std::string>& to avoid copies. Also change the signature to match where you call this function.

Colecf · 2024-04-17T23:55:10Z

src/var.cc

+  std::string referencingFile(loc.filename);
+  bool valid = false;
+  for (const std::string& prefix : prefixes) {
+    if (referencingFile.find(prefix) == 0) {


+1, and if you use HasPrefix you can use loc.filename directly instead of creating a copy of it in referencingFile.

Colecf · 2024-04-18T00:00:12Z

src/func.cc

+  std::stringstream ss(args[1]->Eval(ev));
+  std::string prefix;
+  while (ss >> prefix) {
+    prefixes.push_back(prefix);


There should be some checks here:

The path doesn't start with a / or ../ (isn't absolute)

The path is equal to NormalizePath(path)

One path isn't a prefix of another

Colecf · 2024-04-18T00:09:28Z

testcase/var_visibility_prefix_invalid_file.mk

+VAR2 := $$(BAZ)
+VAR3 = $$(BAZ)
+VAR4 = $($(BAZ))
+VAR5 := $($(BAZ))


VAR3, VAR4, and VAR5 are not being tested because the makefile errored out at VAR2. You should move them into separate tests.

From our discussion, VAR2/VAR3 aren't referenced due to the double dollar sign, and VAR4 isn't because the recursive expansion means it's not expanded.

That VAR4 case is a bug in my eyes, we should also check for usages via recursive expansion. This may require expanding recursive assignments immediately but then throwing away the results.

Colecf · 2024-04-18T00:09:55Z

src/var.cc

+    }
+  }
+  if (!valid) {
+    ERROR("%s is not a valid file to reference variable %s. Line #%d", loc.filename, name, loc.lineno);


Please print out the valid locations the variable can be in as part of the error message.

Colecf · 2024-04-18T00:10:58Z

testcase/var_visibility_prefix_invalid_file.mk

+endif
+
+test:
+	@:


Please add a test that involves multiple makefiles, and a test like:

$(KATI_visibility_prefix BAZ, baz) FOO := BAZ X := $($(FOO)) # should be an error

src/func.cc

lamontj · 2024-04-19T18:02:45Z

src/func.cc

+    if (HasPrefix(prefix, "/")) {
+      ERROR_LOC(ev->loc(), "Visibility prefix should not start with /");
+    }
+    if (HasPrefix(prefix, "../")) {


This allows "./../", which has the same effect. The best solution is probably to just not print normalizedPrefix in line 681, rather than trying to detect all of the places where the passed argument would escape the workspace.

I think this is fine, ./../ would fail the normalization check.

What is the purpose of having this check here? If it was removed completely the normalization check would still fail.

This check is either:

redundant because of the normalization check, or

insufficient in its effort to avoid leaking the parent directory name in the normalization check. If leaking the parent directory is considered bad, then just drop that from the error message below.

In any case, this check provides no added value.

NormalizePath can still produce paths starting with ../. For example, ../foo is a normalized path. But a ../ anywhere else in the path would be removed by the normalization process, e.g. foo/../../bar normalizes to ../bar, so we'd catch it.

I don't think this check is either redundant or insufficient.

It's not really about leaking the parent directory name, it's that you'd never want to make a variable visible to a location outside of the android source tree, so we should deny that nonsense argument.

I was misunderstanding NormalizePath, and expecting "../foo" to turn into an absolute path.

Agreed.

syntax: $(KATI_visibility_prefix var, prefix) 1, Add a func KATI_visibility_prefix that takes a variable name and a list of strings, set this variable's visibility to these strings. Each string represents the relative path from the root, and is considered as prefix. e.g. $(KATI_visibility_prefix VAR, vendor/ device/b baz.mk) --> VAR is visible to "vendor/foo.mk", "device/bar.mk", "device/baz.mk", "baz.mk", but not visible to "bar.mk", "vendor.mk" or "vendor/baz.mk". If variable visibility is set more than once, and with a different list of strings, an error will occur. 2. When a variable is being referenced, if this variable has visibility prefix set, check if the current referencing file matches the visibility prefix. Throw an error if not. 3. In $(KATI_visibility_prefix FOO, prefix) If FOO is not defined, create a variable FOO with empty value. The prefix can also be reference to variable. If so, this function will expand the reference and set the visibility_prefix.

mrziwang requested a review from a team as a code owner April 17, 2024 23:29

lamontj suggested changes Apr 17, 2024

View reviewed changes

Colecf requested changes Apr 18, 2024

View reviewed changes

mrziwang force-pushed the kati-visibility branch 2 times, most recently from 4ddffa7 to 958bd22 Compare April 18, 2024 23:58

Colecf reviewed Apr 19, 2024

View reviewed changes

src/func.cc Outdated Show resolved Hide resolved

mrziwang force-pushed the kati-visibility branch 2 times, most recently from 935f0cd to 7a22bea Compare April 19, 2024 17:49

lamontj reviewed Apr 19, 2024

View reviewed changes

mrziwang force-pushed the kati-visibility branch from 7a22bea to 18ec8a7 Compare April 19, 2024 18:54

mrziwang force-pushed the kati-visibility branch from 18ec8a7 to 238a9f1 Compare April 30, 2024 16:49

Colecf approved these changes Apr 30, 2024

View reviewed changes

Colecf merged commit 8b60db2 into google:master Apr 30, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add func KATI_visibility_prefix #275

Add func KATI_visibility_prefix #275

mrziwang commented Apr 17, 2024

lamontj left a comment

lamontj Apr 17, 2024

Colecf Apr 17, 2024

Colecf Apr 18, 2024

lamontj Apr 18, 2024

Colecf Apr 17, 2024

Colecf Apr 17, 2024

Colecf Apr 18, 2024 •

edited

Loading

Colecf Apr 18, 2024

Colecf Apr 18, 2024

Colecf Apr 18, 2024

Colecf Apr 18, 2024

lamontj Apr 19, 2024

Colecf Apr 19, 2024

lamontj Apr 19, 2024

Colecf Apr 19, 2024

lamontj Apr 19, 2024

+              endif
+              test:
+              	@:

Add func KATI_visibility_prefix #275

Add func KATI_visibility_prefix #275

Conversation

mrziwang commented Apr 17, 2024

lamontj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Colecf Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Colecf Apr 18, 2024 •

edited

Loading