fix: Dependency Resolution on RBE

[jlaxson]

The no-remote-exec is definitely not required. I don't use no-sandbox and it seems to work, not sure if there's a case running locally that you would need it.

shutil.copy copies file metadata. In RBE, the source tree may have read-only permissions (Buildbarn does this, as it hardlinks input files in from a cache). Using .copy means the destination file is then not writable and can't be modified subsequently. copyfile copies the content only, the comment remarking that copyfile was not acceptable is not supported by python documentation.

[chrislovecnm]

You may have caused tests to run that have not run in a while, or we hit a flake. If you push another commit it will rub CI again

[jlaxson]

Is there something required for RBE actions to get network access in CI?

[jlaxson]

@hrfuller @rickeylev Could you take a look please

[rickeylev]

re: removing no-remote-exec: It can't be run remotely, though, because part of what it does is update a local file. If the generation of new content was split from the updating of the local file, then no-remote-exec could just be applied to the local file updater.

Removing no-sandbox is probably fine; I don't see why sandboxing would affect it.

The shutil.copy -> copyfile change: this part is a bit of mystery to us. There is another PR wanting a similar change. But the surrounding comments and originating PR make it sound pretty clear using copyfile didn't work in some cases.

I did some light searching, and I didn't find much to verify that. There are references about this with shutil.move, and some mentions of Python 2. Thinking maybe there was an old shutil.copy bug with Python 2, I lightly searched for that, but didn't see anything that indicated that.

So, I'm not sure whether switching is strictly better, or just trades one broken case for another.

[rickeylev]

re: removing no-remote-exec: It can't be run remotely, though

Ah actually, I think this is OK. If you do bazel run :foo.updater, then it'll run locally, by definition.

If you do bazel build :foo.updater, then it should skip the copy-back-to-local-file part because that won't have BUILD_WORKSPACE_DIRECTORY defined. Running that remotely is fine

[jlaxson]

Update still works without the tags, because you are bazel runing the updater which runs locally, so the tags there are useless. no-remote-exec will never work in a scenario where the host and exec platforms are different (e.g. bazel on macos compiling to linux)

Switching has got to be strictly better. I think the original committer had shutil.copy confused with os.replace/rename. shutil.copy is implemented as shutil.copyfile followed by shutil.copymode, so copyfile must work fine in scenarios where .copy works.

[jlaxson]

Ha you're way ahead of me.

[github-actions[bot]]

This Pull Request has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days. Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_python!

[psalaberria002]

Can someone review this PR? Should not be auto-closed.

[aignas]

My concern is that it uses shutil.copyfile even though there was a comment that shutil.copyfile was not working under certain cirrumstances. It could be that the comment is wrong. I would be happy to see this rebased and merged if someone can explain me the history why and how the comments were written previously.

Once this is rebased we could ask people to test in corporate env.