Repository navigation
Replace afero with the new Go io/fs package #1079
Description
During #1059 it became obvious afero has some problems specifically with it's MemMapFs, but also missing features in CacheOnReadFs, problems with Walk(which are due to the way MemMapFs works in some ways) and possibly many more.
Given that the cleanest (and I would argue very good) solution is for MemMapFs to be rewritten in a way that it fixes all it's bugs and is in general not hacky, I propose that we fork afero and use the go modules replace functionality. We should still try to merge all the fixes in the original repo, but unfortunately it looks pretty dead to me so it will probably take a while ...
This is definitely not high priority, as the current workarounds work, but it will still make the experience of using MemMapFs and afero much less error-prone and with less hacks.
Activity
na-- commentedon Jul 12, 2019
I think the issue should be called "Fork or replace afero", since it might be better if we use another library, instead of trying to fix all of the issues with afero, and then potentially inherit the maintenance burden of the forked library... For example, it might be worth investigating https://github.com/C2FO/vfs, since it seems much more active and it's supposedly inspired by afero. Or maybe https://github.com/twpayne/go-vfs, since it supposedly has afero compatibility. Or maybe this: https://godoc.org/golang.org/x/tools/godoc/vfs - this seems semi-official 😄
But yeah, if we end up sticking with afero and forking it, go modules (#1070) definitely seems like a prerequisite...
na-- commentedon Jan 27, 2021
To update this, it's probably worth waiting for https://tip.golang.org/pkg/io/fs/ and seeing what gets built on top of it
[-]Fork afero[/-][+]Replace afero with the new Go io/fs package[/+]na-- commentedon Feb 18, 2021
Go 1.16 was released:
yorugac commentedon Jan 6, 2022
Since this issue mentions only package transition at the moment, I'd like to add a related suggestion. As part of the work here, if possible it'd be nice to change this map into a struct:
k6/loader/filesystems.go
Lines 44 to 47 in 9d6adcc
Also, is there going to be some kind of a design proposal for this issue? I.e. how interfaces would change with the transition and so on.
mstoykov commentedon Jan 6, 2022
What is the expected benefit of this? Because currently one of the benefits of it being a map is that if we decide to add more protocols you just add them to the map and the rest of code should stay unchanged. I am technically not against it being a struct, just not certain what the benefit is going to be. More type safety?
Not really - whoever works on this will need to go through what
io/fsprovides and what we currently use and bridge the gap somehow. If at that point they want to make a design proposal before publishing (or even working on the PR, if that is possible) - it's fine, but part of the work here is to actually come with what needs to be done, as is the case with most issues ;)yorugac commentedon Jan 6, 2022
That + better readability.
Actually, my question about design was mostly addressed to @olegbespalov since he's assigned here 🙂. I guess I should have tagged him explicitly, sorry for confusion.
olegbespalov commentedon Jan 17, 2022
@yorugac @mstoykov, thanks for commenting!
Changing the map to a struct is also something that I thought about when I was investigating this task. From my side, I add to a struct another property that I believe is important. I'm talking about better maintainability.
I see that the
io/fsfor now provides more readers' interfaces. So I'm not sure it will be possible to fully migrate to theio/fsfrom afero. What still can (and I believe should be done) is refactor towards the smaller interfaces. For example, we mostly pass theafero.Fsdownstream, but do we need the whole filesystem abstraction in all places? 🤔Also, if it stays the way it's right now, the task itself seems to be a bit vague 🤔 Maybe it's possible to list our main pain points somehow or if it's not only the bugs but also the whys we want to migrate.
For example:
afero.Fsis too huge and replacing it with something tinier makes the design better. It improves testing possibilities. Makes code more flexible and improves maintainability.mstoykov commentedon Jan 27, 2022
Even now we have code on top of
afero.FSand we definitely will need at least part of it. The idea here though is that we won't use a dependency that we have issues with, which we need to work around.Addionally the places where we write to the afero.Fs are :
fileandhttpsfilesystem have the same structure:cachingFS->realFsReadingFromSomewherewhere we in practice don't have the second case when running from an archive.This definitely isn't a small amount of work, but arguably most of the changes will be ... in the tests :(.
Probably not, we do use quite little of it directly either way. By which I mean that we don't need the whole
afero.FSinterface, but we likely always need a whole "filesystem".the main point is we have problems with afero and it's not maintained. The issue isn't really ... high priority as we also don't really change this part of the code, but when we have to, different bugs in afero make it harder to be certain one of the many afero.FS implementations won't break something.
I guess we also don't need 90% of what it gives us. Along the way
io/fswas introduced which gave us at least a basis for a solution so at least we don't have to spin a whole knew library on our own (hopefully 🤞 ).As with most "we don't like this we probably should change it" issues, specifying how the final solution will work is 90% of the work.
I do think that it might be better if we(you :P) first remove all usages of afero outside of
lib/fsextand use only fsext everywhere and then try to remove afero fromlib/fsextas well. This likely will :aferoand what needs to be written to getio/fsto do the work.io/fssolutions if for some reason that is preferable.p.s. From my memories of this code (and some recent going through parts of it) - there are a lot of hairy places, unfortunately, so doing this in multiple passes/PRs will definitely be preferable as breaking any minor corner case here has big consequences and while there are a ton of tests, you will likely need to touch most of those as well :(
Hide afero behind fsext package
Hide afero behind fsext package