UX optimization: reversed patch == hung fetch...
sean- opened this issue · 5 comments
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).
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.
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.
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.
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
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/ansi@v0.0.0-20200706080929-d51e80ef957d",
"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.