check owner and group executable bits

[zbirenbaum]

Description

Changes the running action manager to check the owner and group executable permissions of artifacts instead of just others. Prevents files from losing executable status when being copied from the worker to the CAS.

Fixes #675

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

This includes a regression test for losing executable permission when copying from worker to CAS.

Checklist

• Updated documentation if needed
• Tests added/amended
• bazel test //... passes locally

---

This change is 

[allada]

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: pre-commit-checks

a discussion (no related file):
nit: Please correct the paragraph part in the pull request to be grammatically correct.

---

a discussion (no related file):
Need a test

---

[zbirenbaum]

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: pre-commit-checks

a discussion (no related file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit: Please correct the paragraph part in the pull request to be grammatically correct.

Haha nice catch! Just fixed it

---

a discussion (no related file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

Need a test

Got it, I'll work on that next

---

[allada]

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: pre-commit-checks

a discussion (no related file):

Previously, zbirenbaum (Zach Birenbaum) wrote…

Haha nice catch! Just fixed it

Nice, but please remove the question now. This will become your commit message.

---

[zbirenbaum]

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: pre-commit-checks

a discussion (no related file):

Previously, zbirenbaum (Zach Birenbaum) wrote…

Got it, I'll work on that next

Test has been added and I confirmed that it fails without the change

---

[adam-singer]

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: pre-commit-checks

---

-- commits line 5 at r2:
commits should be squashed

[allada]

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: pre-commit-checks

---

-- commits line 4 at r3:
nit: should be in form:

Short tile without trailing period

Short paragraph of what changed.

[fixes|closes|towards] #ticket_number

---

nativelink-worker/tests/running_actions_manager_test.rs line 145 at r3 (raw file):

#[cfg(test)]
mod running_actions_manager_tests {
    use nativelink_proto::build::bazel::remote::execution::v2::command::EnvironmentVariable;

nit: Put this at top of file with other imports.

---

nativelink-worker/tests/running_actions_manager_test.rs line 2586 at r3 (raw file):

        fs::create_dir_all(&root_work_directory).await?;

        let running_actions_manager = Arc::new(RunningActionsManagerImpl::new_with_callbacks(

nit: I believe in this test we can use RunningActionsManagerImpl::new unless you need those callbacks.

---

nativelink-worker/tests/running_actions_manager_test.rs line 2618 at r3 (raw file):

                    "sh".to_string(),
                    "-c".to_string(),
                    "echo whoami > ${OUTS} && chmod 740 ${OUTS}".to_string(),

nit:

"touch {FILE_1_NAME} && chmod 700 {FILE_1_NAME}".to_string()

And remove the environ.

---

nativelink-worker/tests/running_actions_manager_test.rs line 2650 at r3 (raw file):

                            ..Default::default()
                        }),
                        salt: SALT,

nit: Inline these. Or better yet use ..Default::default() and remove them.

---

nativelink-worker/tests/running_actions_manager_test.rs line 2659 at r3 (raw file):

        };
        // Ensure the file copied from worker to CAS is executable.
        assert!(action_result.output_files[0].is_executable);

nit: Add description of error as second argument.

[zbirenbaum]

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: pre-commit-checks

---

-- commits line 5 at r2:

Previously, adam-singer (Adam Singer) wrote…

commits should be squashed

Done.

[allada]

Reviewed 1 of 2 files at r5.
Reviewable status: 1 of 1 LGTMs obtained

[allada]

Dismissed @adam-singer from a discussion.
Reviewable status: complete! 1 of 1 LGTMs obtained

[adam-singer]

Reviewed 1 of 2 files at r5, all commit messages.
Reviewable status: complete! 2 of 1 LGTMs obtained

[zbirenbaum]

Reviewed 1 of 1 files at r1, 1 of 1 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: complete! 2 of 1 LGTMs obtained