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

zip: Add fs.FS support for Go 1.16+ #432

Merged
merged 9 commits into from Sep 6, 2021
Merged

Conversation

klauspost
Copy link
Owner

@klauspost klauspost commented Sep 1, 2021

Change writer api to match stdlib as well.

func (w *Writer) Copy(name string, src *File) error is changed to func (w *Writer) Copy(src *File) error to match the stdlib.

@orijbot
Copy link

orijbot commented Sep 1, 2021

Change writer api to match stdlib as well.
@klauspost klauspost merged commit 2d75f68 into master Sep 6, 2021
@klauspost klauspost deleted the upgrade-zip-support branch September 6, 2021 08:35
@odeke-em
Copy link

odeke-em commented Sep 6, 2021

Well done @klauspost, improvements for the benchmarks you care about https://dashboard.github.orijtech.com/benchmark/c099d0acb095438bb5590e89b746a405 :-)

If I may kindly ask, for notifications, would you like a comment for every commit telling you when it completed? Or would you prefer the Github Actions style notification that you have to proactively look at for completion on the PR's page? /cc @cuonglm @kirbyquerby

@klauspost
Copy link
Owner Author

@odeke-em Yeah. This seems like a good example of non-changes yielding rather big fluctuations. None of the benchmarks are affected by the code change in this PR. Kind of frustrating that the Go compiler has so big variance in compiles.

I definitely would like a notification. If possible with a table. Some systems like this will delete previous results when results from a new push arrives. That would be great so history doesn't get too cluttered with outdated benchmarks.

/cc @cuonglm @kirbyquerby

@saracen
Copy link
Contributor

saracen commented Nov 1, 2021

% go version 
go version go1.13.8 darwin/amd64
% go mod tidy
...
	github.com/klauspost/compress/zip imports
	io/fs: malformed module path "io/fs": missing dot in first path element

It looks like go mod tidy doesn't take into consideration build tags. This is a problem for us, as we use go mod tidy on the CI to ensure consistency.

@klauspost I don't expect there's anything that can be done about this, but figured I'd leave this here for anybody else that stumbles across this problem.

@klauspost
Copy link
Owner Author

@saracen Minimum supported Go version is currently Go 1.15 - could you check on that?

@saracen
Copy link
Contributor

saracen commented Nov 1, 2021

@klauspost That does appear to work. I figured the same thing would happen as io/fs was only introduced in go1.16. Apologies for the noise!

klauspost added a commit that referenced this pull request Jun 9, 2022
Accidentally reverted #313 in #432

Fixes #623
@klauspost klauspost mentioned this pull request Jun 9, 2022
klauspost added a commit that referenced this pull request Jun 9, 2022
Accidentally reverted #313 in #432

Fixes #623
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

Successfully merging this pull request may close these issues.

None yet

4 participants