allow patching to stdlib

[hunshcn]

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

allow patching to stdlib. maybe a unreleased bug fix

Which issues(s) does this PR fix?

None

Other notes for review

[fmeum]

From a usability perspective, do we want to allow patching on go_download_sdk if no version is pinned? I could see that making patches brittle - we could add a fail for that case.

[fmeum]

If we add patches, we should also add patch_prefix. Could you add that and move these attributes above the implicit _sdk_build_file attribute?

[hunshcn]

I don't understand the meaning of patch_prefix. Could you explain it?

[fmeum]

Sorry about that, the attribute I suggested doesn't exist. What I really meant is patch_args (see the same attribute on http_archive), which can be used to set the length of the segment that should be stripped from patch paths via -p.

[hunshcn]

I have moved attributes to _sdk_build_file.

[hunshcn]

all done @fmeum

[hunshcn]

@fmeum why #3684 ?

[fmeum]

Sorry, I just forgot about this PR!
@tyler-french Let's compare and combine the best of both.

[tyler-french]

I created #3698 because I couldn't figure out how to update this PR.

I think the only change I made was to remove patch_args in favor of patch_strip. I have only ever seen the args used to pass p1 or p0, so moving to the patch_strip will hopefully limit abuse of this, which is also how Bzlmod was designed: https://docs.bazel.build/versions/5.0.0/skylark/lib/globals.html#archive_override

[tyler-french]

@hunshcn FYI the difference between the two PRs was that #3684 was intended for the Bzlmod extension, whereas this one is focused on WORKSPACE usage. I think for now I will keep the flags added to this PR restricted to workspace, and keep the Bzlmod go_deps.download tag to only have access to patches and patch_strip.

Let me know what you think @fmeum!

[hunshcn]

@tyler-french i think the usage of patch_args in this patch function should be similar to the patch_args in go_repository.

https://github.com/bazelbuild/bazel-gazelle/blob/master/repository.md#go_repository

Since this pr is for the workspace, it can be merged only by resolving the conflict, but the parameter form of patch_args needs to be discussed. @fmeum

[tyler-french]

We should be as restrictive as possible since we will need to maintain all uses in the long term.

It seems a bit improper to allow local_sdk and wrap_sdk to patch the SDK. I restricted #3698 to only allow patching on the go_download_sdk tag.

@tyler-french i think the usage of patch_args in this patch function should be similar to the patch_args in go_repository.

I think it would be best to limit the users rather than follow this design pattern. FWIW go_repository rules should probably have been designed with just patch_strip arguments. I also think patch_strip is more clear and user friendly.

We also need to maintain this into the future, and being more restricted here makes it more maintainable since we know the user can only modify the strip. To help with this, I removed the patch_cmd and patch_tool since these are dangerous and allow users to shell out to their machines, which we should probably not support.

Also, do you have examples for usage of the remote_patches? I will need to add tests for these features I think before we add them, similar with patch_cmds_win, we will need to add a test for the Windows tests here to use this.

However, I am happy to go in either direction. Maybe @linzhp if you have any opinions.

[linzhp]

I agree that there is no need to allow allow local_sdk and wrap_sdk to patch the SDK:

• wrap_sdk takes a list of platform-specific SDKs, which are often downloaded by other repository rules like http_archive. Patches can be applied on those repository rules instead.
• local_sdk takes a local path. Patches can be applied to the local paths instead.

With regards to the patch attributes, I don't have strong opinion on either direction. On one hand, they are consistent with http_archive in WORKSPACE; on the other hand, bzlmod prefers more restrictive attributes like patch_strip. Since WORKSPACE is going to be deprecated, I am leaning towards the consistency with bzlmod.

[hunshcn]

I agree that there is no need to allow allow local_sdk and wrap_sdk to patch the SDK:

• wrap_sdk takes a list of platform-specific SDKs, which are often downloaded by other repository rules like http_archive. Patches can be applied on those repository rules instead.
• local_sdk takes a local path. Patches can be applied to the local paths instead.

With regards to the patch attributes, I don't have strong opinion on either direction. On one hand, they are consistent with http_archive in WORKSPACE; on the other hand, bzlmod prefers more restrictive attributes like patch_strip. Since WORKSPACE is going to be deprecated, I am leaning towards the consistency with bzlmod.

Api consistency is a very important thing, should not be broken.

[hunshcn]

@fmeum action need

[fmeum]

I took some time to think this through:

1. patch_args and patch_cmd* result in a call to either the host's patch utility or a shell command, both of which add host dependencies and may not be portable. We should discourage this (and point to patch_strip, sorry for mixing this up in an earlier comment), but if users need it, they could still rely on an http_archive with these attributes and wrap it with go_wrap_sdk. This saves us from having to maintain parity with http_archive and emphasizes composition of rules instead of "do it all" rule design. We should document how you can use go_wrap_sdk for this purpose.
2. As far as I know remote_patches was added mostly to satisfy the needs of Bzlmod, which uses it internally for module repos populated from registry contents. I don't really see a common use case for end users. At the very least we should wait for multiple users asking for this functionality before we consider adding it.

[hunshcn]

I took some time to think this through:

1. patch_args and patch_cmd* result in a call to either the host's patch utility or a shell command, both of which add host dependencies and may not be portable. We should discourage this (and point to patch_strip, sorry for mixing this up in an earlier comment), but if users need it, they could still rely on an http_archive with these attributes and wrap it with go_wrap_sdk. This saves us from having to maintain parity with http_archive and emphasizes composition of rules instead of "do it all" rule design. We should document how you can use go_wrap_sdk for this purpose.
2. As far as I know remote_patches was added mostly to satisfy the needs of Bzlmod, which uses it internally for module repos populated from registry contents. I don't really see a common use case for end users. At the very least we should wait for multiple users asking for this functionality before we consider adding it.

So your suggestion is not to merge and keep the status quo, right?

[fmeum]

So your suggestion is not to merge and keep the status quo, right?

That also depends on your original motivation to propose this change. I would like to learn more about your use case and which change would allow you to do something that otherwise wouldn't be possible. For example, it would be interesting to see whether the indirection through go_wrap_sdk on an http_archive would work for you.

[hunshcn]

actually,

453134b

Actually, I originally just wanted to add the patch capability to go_download_sdk 461EBCC. At your suggestion (add patch_prefix and move to _sdk_build_file), I modified the implementation.

But #3684 was merged, which I don't think is in line with the initial discussion.

At the same time, it is not elegant for the api usage in WORKSPACE (patch_strip, even this is int)