Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UX optimization: reversed patch == hung fetch... #3052

Open
sean- opened this issue Jan 22, 2024 · 5 comments
Open

UX optimization: reversed patch == hung fetch... #3052

sean- opened this issue Jan 22, 2024 · 5 comments

Comments

@sean-
Copy link
Contributor

sean- commented Jan 22, 2024

This is embarrassing, and it took entirely too long to figure out, IMO, but I had a custom patch reversed (i.e., diff -u new_file old_file). Behind the scenes patch was asking:

Reversed (or previously applied) patch detected!  Assume -R? [y]

Could the behavior of patch be tweaked to pass the -f or -N (or maybe -t, but I think this would be problematic to debug for different reasons)? I'd rather have things fail fast than hang on user input and be hung/stalled. Needless to say, I thought this was a VPN or WiFi issue while flying, not something stuck on user input because it was hung "Fetching". From a UX perspective, having the output updated to be "Patching" vs "Fetching" would've also been a sufficient clue.

Feel free to close, but I think this would be an easy UX win when someone does something dumb (i.e., flying and hacking while sleep-deprived).

@Tatskaari
Copy link
Member

Tatskaari commented Jan 22, 2024

I agree, we shouldn't be hanging on interactive input like this. Do you know what these flags do? I think we essentially want the default behavior (i.e. don't automatically reverse it for us) but when it detects this case, it should just error. From this description here, I assume -t would have applied this patch, a bit like if you passed -R:

  -t  --batch  Ask no questions; skip bad-Prereq patches; assume reversed.
  -f  --force  Like -t, but ignore bad-Prereq patches, and assume unreversed.

From a UX perspective, having the output updated to be "Patching" vs "Fetching" would've also been a sufficient clue.

This is fairly easy to do but has a performance hit as patches are often used to patch large repos. We'd have to move patching to it's own target so it can have a different building description. This means we have to copy the files into a temp directory, and adding more intermediate targets means we have to repeat this step for each target.

@sean-
Copy link
Contributor Author

sean- commented Jan 22, 2024

I'd use -f. If the patch fails to apply, it should be an error (a reversed patch shouldn't magically apply because this is a form of non-determinism). In my case, the -f would've caught this error and it would've been obvious there was a patch problem, not something blocked waiting for input on stdin.

@Tatskaari
Copy link
Member

Yeah, that sounds like the right thing to do. Which build defs are you experiencing this with? There are a number of places where we patch things in Please.

@sean-
Copy link
Contributor Author

sean- commented Jan 23, 2024

go_module():

go_module(
    name = "tengo",
    install = [
        "",
        "parser",
        "stdlib",
        "stdlib/json",
        "token",
    ],
    module = "github.com/d5/tengo/v2",
    patch = "d5-tengo-recover.patch",
    version = "v2.16.1",
)

d5/tengo#439, for reference

@sean-
Copy link
Contributor Author

sean- commented Apr 15, 2024

Here's a new variation on an old theme: patching a file that already exists. I even run into this stuck issue when performing a plz clean -f:

# get https://proxy.golang.org/github.com/mgutz/ansi/@v/v0.0.0-20200706080929-d51e80ef957d.zip: 200 OK (0.011s)
{
	"Path": "github.com/mgutz/ansi",
	"Version": "v0.0.0-20200706080929-d51e80ef957d",
	"Query": "d51e80e",
	"Info": "/Users/seanc/src/myrepo/plz-out/tmp/third_party/go/_ansi#dl._build/go_mod_download_folder/pkg/mod/cache/download/github.com/mgutz/ansi/@v/v0.0.0-20200706080929-d51e80ef957d.info",
	"GoMod": "/Users/seanc/src/myrepo/plz-out/tmp/third_party/go/_ansi#dl._build/go_mod_download_folder/pkg/mod/cache/download/github.com/mgutz/ansi/@v/v0.0.0-20200706080929-d51e80ef957d.mod",
	"Zip": "/Users/seanc/src/myrepo/plz-out/tmp/third_party/go/_ansi#dl._build/go_mod_download_folder/pkg/mod/cache/download/github.com/mgutz/ansi/@v/v0.0.0-20200706080929-d51e80ef957d.zip",
	"Dir": "/Users/seanc/src/myrepo/plz-out/tmp/third_party/go/_ansi#dl._build/go_mod_download_folder/pkg/mod/github.com/mgutz/[email protected]",
	"Sum": "h1:5PJl274Y63IEHC+7izoQE9x6ikvDFZS2mDVS3drnohI=",
	"GoModSum": "h1:01TrycV0kFyexm33Z7vhZRXopbI8J3TDReVlkTgMUxE="
}
patching file go.mod
Patch creates file that already exists!  Assume -R? [y]
    ///third_party/go/github.com_mgutz_ansi//:installs
///third_party/go/github.com_mgutz_ansi//:installs: failed to build subrepo

With the BUILD.plz file for third_party/go/BUILD.plz:

go_repo(
    name = "ansi",
    install = ["."],
    module = "github.com/mgutz/ansi",
    patch = ["ansi-go.mod.patch"],
    version = "d51e80e",
)

and the patch:

diff --git a/go.mod b/go.mod
new file mode 100644
index 0000000..abcabfe
--- /dev/null
+++ b/go.mod
@@ -0,0 +1,9 @@
+module github.com/mgutz/ansi
+
+go 1.22.2
+
+require (
+	github.com/mattn/go-colorable v0.1.13 // indirect
+	github.com/mattn/go-isatty v0.0.16 // indirect
+	golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab // indirect
+)
diff --git a/go.sum b/go.sum
new file mode 100644
index 0000000..47ebc10
--- /dev/null
+++ b/go.sum
@@ -0,0 +1,6 @@
+github.com/mattn/go-colorable v0.1.13 h1:fFA4WZxdEF4tXPZVKMLwD8oUnCTTo08duU7wxecdEvA=
+github.com/mattn/go-colorable v0.1.13/go.mod h1:7S9/ev0klgBDR4GtXTXX8a3vIGJpMovkB8vQcUbaXHg=
+github.com/mattn/go-isatty v0.0.16 h1:bq3VjFmv/sOjHtdEhmkEV4x1AJtvUvOJ2PFAZ5+peKQ=
+github.com/mattn/go-isatty v0.0.16/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/yFXSvRLM=
+golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab h1:2QkjZIsXupsJbJIdSjjUOgWK3aEtzyuh2mPt3l/CkeU=
+golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=

Which has made merging #3053 important, imo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants