Can one of the admins verify this patch?

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. 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, please reply here (e.g. I signed it!) and we'll verify. Thanks.

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
• In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

I signed it!

CLAs look good, thanks!

Jenkins: test this please.

This looks reasonable to me.

jenkins retest this please.

Bootstrapping fails on darwin because wrapped_clang does not have the x mode now...

jenkins retest this please.

@damienmg Do we have any testing of the impact of this on rules_docker? I suspect this could be breaking to some users.

Don't we have the dep in internal testing?

The docker test where still there and did not broke, they got removed this morning though.

[jenkins] test this please

@damienmg We haven't unforked build_tar.py in rules_docker because it takes a Bazel release cycle before the upstreamed changes are available. :(

Anything I can do here? Having trouble understanding what failed? Seems like something related to freebsd?

So the distribution artifact does not add +x flag to tools/cpp/clang_wrapper (or something like that) and so it fails to executed where it is used (freebsd and darwin). Just set tools/cpp/clang_wrapper should fix the issue.

Can this get tested again?

jenkins: test this please

jenkins: retest this please

FTR it passes all our tests internally so it should be merged in the following days. Sorry for taking so long.

Thanks, I definitely still need this so let me know if I can do anything. Everything seems to pass when I run bazel test locally but not sure if I was doing something wrong

An update: this needs to fix some project internally, so it will take some time before I can get to it.

Friendly ping @damienmg, are internal changes done?

Can one of the admins verify this patch?

No, see cl/168508781

Any update on this?

Hello,

Unfortunately I had no time to do the work yet. We have the Bazel Conference next week so it is unlikely I can do that in the following weeks.

Sorry for such delays.

I had one thought that could make the import simple: have an option to build_tar (on by default) that maintains the old permission settings (pkg_tar would deactivate that). The problems is with the interaction with rules_docker, but if default behavior of build_tar doesn't change, it should be possible to import it directly.

Reassigning to Marcel as I am leaving the Bazel team.

I'm still interested in this and can do the work. Like suggested above, maybe we can change the functionality of the "mode" parameter so that if it sees the work "keep" it will keep the current permissions on the filesystem. It won't be the default but can be opt-in. That way the docker rules arn't broken and the default behavior doesn't change.

Can one of the admins verify this patch?

ping @mhlopko -- what's the next step here?

We don't rerun actions if only the file system permissions change, making this change a bit risky. I'd recommend to have the default be 0555 and providing a mode to keep the fs permissions, as suggested above, with a big warning that Bazel does not rebuild on permissions-only changes.

@ulfjack and @zlalanne : please advise what we should do with this PR -- merge, rework, or abandon?

Maybe we can abandon this one. I've started to just use the "mode" parameter in pkg_tar explicitly to set exact permissions for all files I'm packaging.

Ok, thank you for your contribution!

The current mode implementation is not intuitive. It's disappointing to see the change get worked so far only to be closed.

If pkg extraction under Bazel needs a particular set of permissions, it can (and probably already does) do that part. The tarball should grab the permissions as they are by default--but at least having the option to do that would be sufficient.

My use case is that I'm using pkg_tar to put files into a container_image and then doing various checks on the imported files to gather feedback for developers. Feedback like "This source file should not be executable." Unfortunately, that test is not something I can do inside the container (without writing a custom rule).