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.

Sign up for GitHub

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

Jump to bottom

archive/tar: decide before go1.22 whether the FileInfoNames interface is Unix-only use #65245

Closed
qiulaidongfeng opened this issue Jan 24, 2024 · 11 comments
Closed

archive/tar: decide before go1.22 whether the FileInfoNames interface is Unix-only use #65245

qiulaidongfeng opened this issue Jan 24, 2024 · 11 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.22

Comments

@qiulaidongfeng
Copy link
Contributor

qiulaidongfeng commented Jan 24, 2024 •
edited

Update : #65245 (comment) And #65245 (comment)

#50102 add FileInfoNames interface to archive/tar
I in August 2023 by https://go-review.googlesource.com/c/go/+/514235 wrote an implementation for the proposal

(My CL only has a complete implementation for changing FileInfoHeader behavior on unix.)

See https://pkg.go.dev/os/user#User , seems that there is no guarantee that uid and gid for non Unix systems are of type int.

This need to be resolved before go1.22 is released, in case it cannot be resolved in a backward compatible way in the future.

Possible solutions:

  1. Modify https://github.com/golang/go/blob/master/src/archive/tar/common.go#L645C1-L647C45 and https://github.com/golang/go/blob/master/src/archive/tar/common.go#L742C1-L743C34, said change FileInfoHeader behaviors on Unix only. This is the easiest.
  2. Delete the existing FileInfoNames interface in go1.22 .
@qiulaidongfeng qiulaidongfeng mentioned this issue Jan 24, 2024
Closed
@gopherbot
Copy link

Change https://go.dev/cl/556399 mentions this issue: doc/go1.22: make the description of FileInfoNames conform to the implementation

@qiulaidongfeng
Copy link
Contributor Author

cc @dsnet because https://dev.golang.org/owners

@cherrymui cherrymui added release-blocker NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 24, 2024
@cherrymui cherrymui added this to the Go1.22 milestone Jan 24, 2024
@qiulaidongfeng
Copy link
Contributor Author

More information:
The FileInfoNames interface added to #50102 is proposed here : #50102 (comment)

The reason this interface only changed FileInfoHeader behavior on unix was that we didn't know how non-Unix systems like windows could get uid and gid from fs.FileInfo.

I just reviewed #50102, and it seems that this interface is designed to avoid any id ->name lookups.
The behavior of any id ->name lookups only exists in Unix systems.

Looks like that's what we're going to do here:

Modify https://github.com/golang/go/blob/master/src/archive/tar/common.go#L645C1-L647C45 and https://github.com/golang/go/blob/master/src/archive/tar/common.go#L742C1-L743C34, said change FileInfoHeader behaviors on Unix only. This is the easiest.

I'll send a CL later to do that.

My understanding of the non Unix uid and gid only from https://pkg.go.dev/os/user#User.
From what I can tell, this interface is unlikely to change the behavior of FileInfoHeader on non-Unix systems.
If you disagree, please comment as soon as possible.
This is because once go1.22 is released there will be no way to modify the FileInfoNames interface in a backward-compatible way.

qiulaidongfeng added a commit to qiulaidongfeng/go that referenced this issue Jan 24, 2024
@qiulaidongfeng
archive/tar: make it clear FileInfoNames change FileInfoHeader behavi…
e50548d 
…or only in unix

fix invalid documentation links incidentally.

For golang#65245

Change-Id: I58ebdb22d6051f676f38e7bed8bc8d9fddcbf6b4
@qiulaidongfeng qiulaidongfeng mentioned this issue Jan 24, 2024
Closed
@gopherbot
Copy link

Change https://go.dev/cl/558135 mentions this issue: archive/tar: make it clear FileInfoNames change FileInfoHeader behavior only in unix

@qiulaidongfeng
Copy link
Contributor Author

qiulaidongfeng commented Jan 24, 2024 •
edited

Sorry, my description of the implementation wasn't quite right.
I now realize that because loadUidAndGid is only implemented on unix systems, FileInfoNames would provide the wrong semantics for non-Unix systems. Unless fs.FileInfo.Sys() provides tar.Header, FileInfoNames.Uname and FileInfoNames.Gname simply receive uid and gid of 0.

@qiulaidongfeng
Copy link
Contributor Author

I suggest FileInfoNames changing FileInfoHeader behavior on unix only at go1.22 .
Because it seems that this interface is designed to avoid any id ->name lookups.
The behavior of any id ->name lookups only exists in Unix systems.

@qiulaidongfeng qiulaidongfeng mentioned this issue Jan 24, 2024
Closed
@bcmills
Copy link
Contributor

bcmills commented Jan 24, 2024

The os/user.User struct uses string values for the Uid and Gid fields to allow it to be implemented portably. It seems like we should probably do the same for the FileInfoNames interface.

Since this is a new feature in Go 1.22 and we're very deep into the release cycle at this point, we should probably roll back the API addition and send it back through proposal review to revise the interface.

@gopherbot
Copy link

Change https://go.dev/cl/558295 mentions this issue: Revert "archive/tar: add FileInfoNames interface"

@gopherbot
Copy link

Change https://go.dev/cl/557840 mentions this issue: doc/go1.22: remove archive/tar.FileInfoNames

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 24, 2024
gopherbot pushed a commit that referenced this issue Jan 24, 2024
@cherrymui
Revert "archive/tar: add FileInfoNames interface"
5000b51 
This reverts CL 514235. Also reverts CL 518056 which is a followup
fix.

Reason for revert: Proposal #50102 defined an interface that is
too specific to UNIX-y systems and also didn't make much sense.
The proposal is un-accepted, and we'll revisit in Go 1.23.

Fixes (via backport) #65245.
Updates #50102.

Change-Id: I41ba0ee286c1d893e6564a337e5d76418d19435d
Reviewed-on: https://go-review.googlesource.com/c/go/+/558295
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
gopherbot pushed a commit that referenced this issue Jan 24, 2024
@cherrymui
doc/go1.22: remove archive/tar.FileInfoNames
02a0927 
CL 514235 is reverted.

Updates #65245.
Updates #61422.

Change-Id: Ib5d2e16c982ab25c8a87af2bdaee8568446cf599
Reviewed-on: https://go-review.googlesource.com/c/go/+/557840
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@gopherbot
Copy link

Change https://go.dev/cl/558296 mentions this issue: [release-branch.go1.22] Revert "archive/tar: add FileInfoNames interface"

gopherbot pushed a commit that referenced this issue Jan 24, 2024
@cherrymui
[release-branch.go1.22] Revert "archive/tar: add FileInfoNames interf…
333ecd4 
…ace"

This reverts CL 514235. Also reverts CL 518056 which is a followup
fix.

Reason for revert: Proposal #50102 defined an interface that is
too specific to UNIX-y systems and also didn't make much sense.
The proposal is un-accepted, and we'll revisit in Go 1.23.

Fixes #65245.
Updates #50102.

Change-Id: I41ba0ee286c1d893e6564a337e5d76418d19435d
Reviewed-on: https://go-review.googlesource.com/c/go/+/558295
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
(cherry picked from commit 5000b51)
Reviewed-on: https://go-review.googlesource.com/c/go/+/558296
@gopherbot
Copy link

Closed by merging 333ecd4 to release-branch.go1.22.

ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
@cherrymui @ezz-no
Revert "archive/tar: add FileInfoNames interface"
b49be69 
This reverts CL 514235. Also reverts CL 518056 which is a followup
fix.

Reason for revert: Proposal golang#50102 defined an interface that is
too specific to UNIX-y systems and also didn't make much sense.
The proposal is un-accepted, and we'll revisit in Go 1.23.

Fixes (via backport) golang#65245.
Updates golang#50102.

Change-Id: I41ba0ee286c1d893e6564a337e5d76418d19435d
Reviewed-on: https://go-review.googlesource.com/c/go/+/558295
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
@cherrymui @ezz-no
doc/go1.22: remove archive/tar.FileInfoNames
15b7f1b 
CL 514235 is reverted.

Updates golang#65245.
Updates golang#61422.

Change-Id: Ib5d2e16c982ab25c8a87af2bdaee8568446cf599
Reviewed-on: https://go-review.googlesource.com/c/go/+/557840
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
Release Status & Release Blockers
Status: Done
Milestone
Go1.22
Development

No branches or pull requests
5 participants
@dmitshur @bcmills @gopherbot @cherrymui @qiulaidongfeng