Add NodeProperty message to filetype nodes #91

traveltissues · 2019-08-15T13:16:30Z

closes #90

If there's interest in this change it can be signed off under the Codethink Ltd cla

googlebot · 2019-08-15T13:16:34Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

googlebot · 2019-08-16T12:13:41Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2019-08-16T12:23:12Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

build/bazel/remote/execution/v2/remote_execution.proto

build/bazel/remote/execution/v2/nodeproperties.md

ola-rozenfeld

LGTM, just a few naming nits:

build/bazel/remote/execution/v2/remote_execution.proto

bergsieker · 2019-10-02T13:50:21Z

build/bazel/remote/execution/v2/remote_execution.proto

@@ -412,6 +412,10 @@ message Action {
  // If true, then the `Action`'s result cannot be cached, and in-flight
  // requests for the same `Action` may not be merged.
  bool do_not_cache = 7;
+
+  // List of required supported [NodeProperty][build.bazel.remote.execution.v2.NodeProperty]
+  // keys. If a property is not recognized the server returns INVALID_ARGUMENT.


Can we beef out this description a bit? In particular, I think the server should have leeway to interpret these properties how it sees fit. For example, some properties may be applicable to directories but not files (or vice-versa).

I agree the server is ultimately responsible for interpreting those properties but I'm not sure what you're requesting. Do you just want the comment here to make that more explicit?

build/bazel/remote/execution/v2/remote_execution.proto

traveltissues · 2019-10-08T14:53:39Z

thanks @ola-rozenfeld @bergsieker

EdSchouten · 2019-10-22T14:40:58Z

How does one specify properties on the root directory of the input root? Wouldn't it have been smarter to place the properties inside the Directory message itself?

I also wonder how this will impact the size of directory objects, considering that all of this (even names of keys) is done through strings.

EdSchouten · 2019-10-22T14:48:21Z

build/bazel/remote/execution/v2/remote_execution.proto

@@ -573,6 +583,9 @@ message Platform {
 // * The files, directories and symlinks in the directory must each be sorted
 //   in lexicographical order by path. The path strings must be sorted by code
 //   point, equivalently, by UTF-8 bytes.
+// * The [NodeProperties][build.bazel.remote.execution.v2.NodeProprty] of files,


This contains a typo.

ola-rozenfeld · 2019-10-22T14:51:41Z

How does one specify properties on the root directory of the input root? Wouldn't it have been smarter to place the properties inside the Directory message itself?

/facepalm
You are 100% right, this should be in the Directory message! Will send a PR to fix.

I also wonder how this will impact the size of directory objects, considering that all of this (even names of keys) is done through strings.

Yeah, I suspect it will have a big impact, my hope basically is that the need for this will be a rare case, and that clients will use these only as needed.

EdSchouten · 2019-10-22T14:53:34Z

Yeah, I suspect it will have a big impact, my hope basically is that the need for this will be a rare case, and that clients will use these only as needed.

Then my hope is that we don't follow through on what is requested in #99.

ola-rozenfeld · 2019-10-22T14:59:47Z

#102

…ild#91)

traveltissues requested review from agoulti, bergsieker, buchgr, ola-rozenfeld and ulfjack as code owners August 15, 2019 13:16

googlebot added the cla: no Pull requests whose authors are not covered by a CLA with Google. label Aug 15, 2019

traveltissues force-pushed the traveltissues/add-node-properties branch 2 times, most recently from b488979 to 323a322 Compare August 16, 2019 12:13

traveltissues force-pushed the traveltissues/add-node-properties branch from 323a322 to c6ee52e Compare August 16, 2019 12:23

googlebot added cla: yes Pull requests whose authors are covered by a CLA with Google. and removed cla: no Pull requests whose authors are not covered by a CLA with Google. labels Aug 16, 2019

traveltissues force-pushed the traveltissues/add-node-properties branch from c6ee52e to 5a8c9bd Compare September 23, 2019 13:42

traveltissues mentioned this pull request Sep 23, 2019

Add properties for files #90

Closed

juergbi suggested changes Sep 25, 2019

View reviewed changes

traveltissues force-pushed the traveltissues/add-node-properties branch 3 times, most recently from 4580d97 to dbfd7b1 Compare October 2, 2019 13:12

ola-rozenfeld reviewed Oct 2, 2019

View reviewed changes

bergsieker reviewed Oct 2, 2019

View reviewed changes

build/bazel/remote/execution/v2/remote_execution.proto Outdated Show resolved Hide resolved

bergsieker reviewed Oct 2, 2019

View reviewed changes

traveltissues force-pushed the traveltissues/add-node-properties branch from dbfd7b1 to 89124bc Compare October 2, 2019 14:41

ola-rozenfeld approved these changes Oct 2, 2019

View reviewed changes

build/bazel/remote/execution/v2/remote_execution.proto Outdated Show resolved Hide resolved

traveltissues force-pushed the traveltissues/add-node-properties branch 2 times, most recently from adad520 to bd5e0a1 Compare October 2, 2019 20:24

traveltissues added 3 commits October 2, 2019 21:27

Add NodeProperty message to filetype nodes

a48a80e

Add node properties to outputs

cb88734

Advertise supported node properties in execution

fb33a35

Add node properties to the Action

4b128a0

traveltissues force-pushed the traveltissues/add-node-properties branch from bd5e0a1 to 4b128a0 Compare October 2, 2019 20:27

traveltissues mentioned this pull request Oct 2, 2019

Deprecate is_executable #99

Open

bergsieker approved these changes Oct 8, 2019

View reviewed changes

bergsieker merged commit 4f46bfd into bazelbuild:master Oct 8, 2019

traveltissues deleted the traveltissues/add-node-properties branch October 8, 2019 14:53

EdSchouten reviewed Oct 22, 2019

View reviewed changes

santigl pushed a commit to santigl/remote-apis that referenced this pull request Aug 26, 2020

Upgrade the SDK to latest dependency versions and Bazel 1.1. (bazelbu…

eae3851

…ild#91)

Cynical-Optimist mentioned this pull request Oct 14, 2020

Workspaces via CAS buildstream-migration/bst-staging#985

Closed

35 tasks

BuildStream-Migration-Bot mentioned this pull request Feb 3, 2021

Workspaces via CAS apache/buildstream#985

Closed

35 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add NodeProperty message to filetype nodes #91

Add NodeProperty message to filetype nodes #91

traveltissues commented Aug 15, 2019

googlebot commented Aug 15, 2019

googlebot commented Aug 16, 2019

googlebot commented Aug 16, 2019

ola-rozenfeld left a comment

bergsieker Oct 2, 2019

traveltissues Oct 2, 2019

traveltissues commented Oct 8, 2019

EdSchouten commented Oct 22, 2019 •

edited

Loading

EdSchouten Oct 22, 2019

ola-rozenfeld commented Oct 22, 2019

EdSchouten commented Oct 22, 2019

ola-rozenfeld commented Oct 22, 2019

Add NodeProperty message to filetype nodes #91

Add NodeProperty message to filetype nodes #91

Conversation

traveltissues commented Aug 15, 2019

googlebot commented Aug 15, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

googlebot commented Aug 16, 2019

googlebot commented Aug 16, 2019

ola-rozenfeld left a comment

Choose a reason for hiding this comment

bergsieker Oct 2, 2019

Choose a reason for hiding this comment

traveltissues Oct 2, 2019

Choose a reason for hiding this comment

traveltissues commented Oct 8, 2019

EdSchouten commented Oct 22, 2019 • edited Loading

EdSchouten Oct 22, 2019

Choose a reason for hiding this comment

ola-rozenfeld commented Oct 22, 2019

EdSchouten commented Oct 22, 2019

ola-rozenfeld commented Oct 22, 2019

EdSchouten commented Oct 22, 2019 •

edited

Loading