Artifact executable bit lost

[lukts30]

E.g

genrule(
    name = "my_file",
    outs = ["test.sh"],
    cmd =  "echo whoami > $(OUTS); chmod +x $(OUTS)"
)

test.sh will be executable after fetching with bazel only because all artifacts it downloads are set to 0555.
Buck2 on the other hand has no such default. Everything that is not is_executable will be set 644.

With both bazel and buck2 after the test.sh file is copied from the worker to the CAS dir the exec bit is no longer set on the file. Therefore when buck2 fetches test.sh it will set it mode to 644.
Anything buck2 writes through actions.write with is_executable=True will still have the exec bit set, even in the CAS dir.

Not forcing 0555 for every file seems reasonable so for better compatibility with buck2 I suggest not discarding the exec permission.

(I could also be wrong this might need to be fixed in buck2).

[allada]

We do set the execution bit, but I suspect buck2 is likely setting is_executable but and not setting unix_mode.

We can already have a bit for managing this on windows:

nativelink/nativelink-worker/src/running_actions_manager.rs

Line 140 in 5df8a78

unix_mode = Some(unix_mode.unwrap_or(0o444) | 0o111);

I guess we would need to see the exact proto buck2 is sending to understand what action we need to take clearly.

Edit:
In looking over this, I think this line: #[cfg_attr(target_family = "windows", allow(unused_assignments))] (in the link above), should actually be target_family = "unix". I think that was supposed to be handling this case but doing it for windows by accident. Ironically the lint checker caught it, since we have allow(unused_assignments) set, but we missed it.

[allada]

In looking into this, I'm no longer convinced what I stated above is the issue.

I am a bit confused though. You said:

With both bazel and buck2 after the test.sh file is copied from the worker to the CAS dir the exec bit is no longer set on the file. Therefore when buck2 fetches test.sh it will set it mode to 644.

But the permission bits are on the action results. This means that the CAS itself has no knowledge of what is executable and what is not, but the individual AC messages know what the permissions should be.

I do see that we don't ever set the AC's message's node_properties (which is where the unix_mode is set). Is it possible that buck2 only looks at unix_mode and doesn't look at is_executable?

[lukts30]

Nativelink only sets is_executable=true when the executable bit is set for others. I have umask set 0077 therefore is_executable=false which breaks it of course.

nativelink/nativelink-worker/src/running_actions_manager.rs

Line 231 in 37184e8

Ok((metadata.mode() & 0o001) != 0)

Should instead be: Ok((metadata.mode() & 0o111) != 0)

[allada]

Aaah, so you are saying that the file permissions is something like 744 and we should be setting the is_executable flag?

Yeah, this seems appropriate. The proto spec does not say anything about how to deal with unix_mode + is_executable, so it's a bit confusing.

Maybe this is a good one for @zbirenbaum to dip your hands in, seems fairly straight forward.

[zbirenbaum]

@adam-singer and I both tested this out and couldn't reproduce it. Could you try following the readme instructions here https://github.com/adam-singer/buck2-example and see if there is anything inconsistent with the situation you described? After running the kill and clean commands the file will be retrieved from the cache by the local machine and remains executable @lukts30

[allada]

I believe what he's saying is that if a file is 0744 it will not be flagged executable, because we only check the last bit.

We should be checking if any of the bits are executable and setting the is_executable flag if any of them are +x.

[zbirenbaum]

Ah I see now, owner and group permissions are being ignored. 0o111 is the correct mask, nice catch @lukts30!

[lukts30]

Thanks for fixing this.
Sorry for the confusion, indeed my initial reproducer would only work on a system where umask is set to anything ending in 7 (e.g. 0027).

genrule(
    name = "my_file",
    outs = ["test.sh"],
    cmd =  "echo whoami > $(OUTS); chmod +x $(OUTS)"
)

Then test.sh would be created as 0640 and the chmod +x would change it to 0750.