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

x/tools/txtar: implement fs.FS #44158

Open
carlmjohnson opened this issue Feb 8, 2021 · 24 comments
Open

x/tools/txtar: implement fs.FS #44158

carlmjohnson opened this issue Feb 8, 2021 · 24 comments
Labels
NeedsFix Tools
Milestone

Comments

@carlmjohnson
Copy link
Contributor

@carlmjohnson carlmjohnson commented Feb 8, 2021

The implementation would be pretty simple, and it would let you use txtar format for templates and http.FileServers.

@gopherbot gopherbot added this to the Proposal milestone Feb 8, 2021
@carlmjohnson
Copy link
Contributor Author

@carlmjohnson carlmjohnson commented Feb 8, 2021

@carlmjohnson carlmjohnson changed the title proposal: x/tools/txtar: implement fs.FS x/tools/txtar: proposal: implement fs.FS Feb 8, 2021
@gopherbot gopherbot added the Tools label Feb 8, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Feb 9, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Feb 10, 2021

SGTM. No need for a proposal.

@rsc rsc changed the title x/tools/txtar: proposal: implement fs.FS x/tools/txtar: implement fs.FS Feb 10, 2021
@rsc rsc added the NeedsFix label Feb 10, 2021
@rsc rsc removed this from Incoming in Proposals Feb 10, 2021
@carlmjohnson
Copy link
Contributor Author

@carlmjohnson carlmjohnson commented Feb 10, 2021

I’m on paternity leave now so I can only type with my left thumb during the day. (Gotta keep the bottle in my right hand.) I’ll try to send out a CL as soon as I can but it may not be until next month.

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Feb 10, 2021

Hate to be a broken record on this particular thing, but I'm pretty sure thanks to #40067, this can't happen until x/tools no longer supports versions of Go without the fs package available, as "new" standard library packages break go mod tidy on older versions of Go (regardless of build tag usage).

The fs.FS interface isn't exactly huge, so potentially there could be a redeclaration of the types in fs for older versions of Go to allow it to work, but that sounds a little annoying.

(This could certainly be implemented in some other repo/module and work fine, though. golang-migrate has done this for now to support io/fs implementations for migrations, for example.)

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 10, 2021

For this kind of use--implementing the interface--the relevant code can be protected by a build tag.

@carlmjohnson
Copy link
Contributor Author

@carlmjohnson carlmjohnson commented Feb 11, 2021

IIUC, #40067 means that a build tag won’t be able to keep go tidy from failing on older versions of Go. There seems to be no way to maintain compatibility with new packages, as opposed to new identifiers, in the standard library. Is that a correct interpretation, @zikaeroh?

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Feb 11, 2021

Yes, that's correct. My interpretation of Ian's comment was that all of the bits and pieces that make up the io/fs.FS interface could be redeclared to make it possible (it's not all that many), but I may have interpreted that incorrectly. Build tags alone cannot guard against new stdlib package usage when it comes to the eyes of the module code (even if when downloaded, the build would have worked fine).

IMO #40067 is going to gate quite a few users for the much-anticipated io/fs and embed packages (or break people who aren't aware), hence me mentioning it before anyone gets into trouble...

@bcmills
Copy link
Member

@bcmills bcmills commented Feb 11, 2021

Yes, that's correct. It is unfortunately not possible to implement the fs.FS interface without importing the fs package.

Probably the cleanest solution in the short term is to put the txtar implementation of fs.FS in a separate package (perhaps golang.org/x/tools/txtar/txtarfs?), which can then safely import the new io/fs package.

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Feb 11, 2021

Probably the cleanest solution in the short term is to put the txtar implementation of fs.FS in a separate package (perhaps golang.org/x/tools/txtar/txtarfs?), which can then safely import the new io/fs package.

Even a new package won't work, it'd need to be another module; #40067 affects modules, as the tooling refuses to continue once it detects that a module it's downloaded contains a package it can't resolve.

@bcmills
Copy link
Member

@bcmills bcmills commented Feb 11, 2021

@zikaeroh, I believe you are mistaken. Neither go get nor go mod tidy scans unimported packages in dependencies. (go get scans the dependencies of the specific packages named on the command line.)

@bcmills
Copy link
Member

@bcmills bcmills commented Feb 11, 2021

It would mean that users would be unable to run go mod tidy within x/tools when authoring changes to the x/tools module. However, users authoring changes to x/tools itself ought to be testing with at least the most recent release anyway, and if they have it installed for testing, they can use it for go mod tidy as well.

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Feb 11, 2021

I was basing my experience on #40067 (comment), but I can see how that list of packages means I was indirectly importing it. I'll have to make a minimal example to look at the edge cases.

I guess this is overall better than "it's impossible", but this behavior is a little infectious in nature and non-obvious that there's a different set of checks that aren't aware of build tags or new packages.

@carlmjohnson
Copy link
Contributor Author

@carlmjohnson carlmjohnson commented Feb 11, 2021

The separate package thing will only work if nothing imports it, at which point I may as well just publish the package on my own account and wait until Go 1.16 is the lowest supported version to comeback and add it to x/tools.

@josharian
Copy link
Contributor

@josharian josharian commented Feb 18, 2021

I couldn't quite tell what the status was here, and I wanted this for my own purposes, so I put together an extremely quick and dirty bridge package: https://github.com/josharian/txtarfs. I look forward to txtar directly supporting fs.FS in the future.

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Feb 18, 2021

I was wrong about it infecting the whole module (@bcmills was of course correct); x/tools gained a new package for godoc/vfs to io/fs and it hasn't broken my usage of x/tools (so long as I don't try to tidy x/tools itself with Go 1.15 or something), so this could safely go in as something similar.

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Feb 18, 2021

Oh, I'm a liar, that was added to an existing package and I didn't notice when I read the CL, so it doesn't serve as a working example (and probably should be added to #40067...).

@zikaeroh
Copy link
Contributor

@zikaeroh zikaeroh commented Mar 11, 2021

With #44557 fixed and now backported, nothing stops adding io/fs and embed to existing packages anymore (other than waiting for patch release adoption).

@carlmjohnson
Copy link
Contributor Author

@carlmjohnson carlmjohnson commented Mar 12, 2021

If it's possible to make this change now, @josharian do you want to just submit your txtarfs as a CL?

@josharian
Copy link
Contributor

@josharian josharian commented Mar 12, 2021

It's kind of a hack, and it allocates unnecessarily, and it creates a new thing instead of just using the txtar directly as an fs.FS. So I think it's not worth upstreaming—it'd be better to do properly.

@carlmjohnson
Copy link
Contributor Author

@carlmjohnson carlmjohnson commented Mar 12, 2021

I started working on an implementation, and getting the directory stuff right basically requires reimplementing MapFS, so I don't think there's actually that much to be gained by doing it "right". I would change As() to Archive.FS() though.

@josharian
Copy link
Contributor

@josharian josharian commented Mar 12, 2021

I see. Pity.

Feel free to steal code wholesale from txtarfs as you please. If you want to credit me, you can add a "Co-authored-by: " trailer to the git commit.

@carlmjohnson
Copy link
Contributor Author

@carlmjohnson carlmjohnson commented Mar 12, 2021

Now I'm looking at it more, and the directory stuff is a pain, but not insurmountable, and having the Open method on Archive itself would be nice. From could be copied wholesale though.

carlmjohnson added a commit to carlmjohnson/tools that referenced this issue Mar 13, 2021
Fixes #44158

From implementation copied with permission from
https://github.com/josharian/txtarfs/blob/main/txtarfs_test.go
golang/go#44158 (comment)

Co-authored-by: josharian
@carlmjohnson
Copy link
Contributor Author

@carlmjohnson carlmjohnson commented Mar 13, 2021

I added a co-authored credit, but that seems to have made the CLA bot angry 😆

carlmjohnson added a commit to carlmjohnson/tools that referenced this issue Mar 13, 2021
Fixes #44158

From implementation copied with permission from
https://github.com/josharian/txtarfs/blob/main/txtarfs_test.go
golang/go#44158 (comment)

Co-authored-by: Josh Bleecher Snyder <josharian@gmail.com>
carlmjohnson added a commit to carlmjohnson/tools that referenced this issue Mar 15, 2021
Fixes #44158

From implementation copied with permission from
https://github.com/josharian/txtarfs/blob/main/txtarfs_test.go
golang/go#44158 (comment)

Co-authored-by: Josh Bleecher Snyder <josharian@gmail.com>
carlmjohnson added a commit to carlmjohnson/tools that referenced this issue Mar 15, 2021
@carlmjohnson
Copy link
Contributor Author

@carlmjohnson carlmjohnson commented Mar 15, 2021

Okay, I opened a PR without the Co-authored-by comment and now it seems to be accepting the CLA.

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

Successfully merging a pull request may close this issue.

8 participants