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

Use VFS abstraction for all filesystem access #1460

Closed
sqs opened this issue Sep 26, 2015 · 14 comments
Closed

Use VFS abstraction for all filesystem access #1460

sqs opened this issue Sep 26, 2015 · 14 comments

Comments

@sqs
Copy link

sqs commented Sep 26, 2015

Some places in hugo still access the filesystem directly (e.g., via package os) instead of through the hugofs VFSes.

I have a WIP sqs/vfs branch that partially addresses this issue, but there will likely be some loss in functionality (e.g., ExpandSymlinks may not be possible with VFSes...not sure without looking into it further).

@spf13:

Would you accept a PR that makes all FS access go via a VFS in hugofs?

Would you be OK using the more common vfs.FileSystem instead of afero?

Motivation: Using the VFS abstraction everywhere makes it much easier to use hugo as a library.

@bep
Copy link
Member

bep commented Sep 26, 2015

There should be other motivation behind this than it "makes it much easier to use hugo as a library".

What do you base the "more common vfs.FileSystem instead of afero" on?

@sqs
Copy link
Author

sqs commented Sep 26, 2015

To expand on the motivation, I want to run Hugo on-the-fly in my existing Go process to process a large number of untrusted sites. I want to avoid hitting disk (even temp dirs) to reduce the resource consumption and have greater certainty that a malicious site can't write to arbitrary locations on my actual filesystem.

I thought the motivation would be self-explanatory because it appears someone went halfway to making all FS access go via a VFS. I imagine they stopped for lack of time, so I'm proposing to finish it.

I said that vfs.FileSystem is more common because I've heard about a lot more people using it, anecdotally. It appears the data are inconclusive, though: godoc/vfs has 59 importers and afero has 39 importers, but there are many forks and so I can't say for sure. (Also, I know of some implicit interface users of godoc/vfs that aren't explicit importers, such as github.com/kr/fs.) My personal preference would be to go with godoc/vfs because it has Lstat, still seems more popular, it is created by the Go authors, and it's simpler/smaller. But I'm fine sticking with afero.

@spf13
Copy link
Contributor

spf13 commented Sep 26, 2015

It should be really easy to modify Hugo to avoid hitting disk completely.
It would be a two or three line change at
https://github.com/spf13/hugo/blob/master/hugofs/fs.go.

I'm not very familiar with VFS and want to look into it more. I spent a bit
of time looking at it and it looks good. I don't think it's simpler or
smaller though. Afero is already in place and I believe is even easier to
use than VFS.

I see what you are saying, and I could totally misunderstand the current
state of things, but I think with very little effort you could have Afero
do what you want. I don't really understand the motivation to switch to VFS
today.

Steve Francia
http://stevefrancia.com
http://spf13.com
http://twitter.com/spf13

On Sat, Sep 26, 2015 at 5:43 AM, Quinn Slack notifications@github.com
wrote:

To expand on the motivation, I want to run Hugo on-the-fly in my existing
Go process to process a large number of untrusted sites. I want to avoid
hitting disk (even temp dirs) to reduce the resource consumption and have
greater certainty that a malicious site can't write to arbitrary locations
on my actual filesystem.

I thought the motivation would be self-explanatory because it appears
someone went halfway to making all FS access go via a VFS. I imagine they
stopped for lack of time, so I'm proposing to finish it.

I said that vfs.FileSystem is more common because I've heard about a lot
more people using it, anecdotally. It appears the data are inconclusive,
though: godoc/vfs has 59 importers
http://godoc.org/golang.org/x/tools/godoc/vfs?importers and afero has 39
importers http://godoc.org/github.com/spf13/afero, but there are many
forks and so I can't say for sure. (Also, I know of some implicit interface
users of godoc/vfs that aren't explicit importers, such as
github.com/kr/fs.) My personal preference would be to go with godoc/vfs
because it has Lstat, still seems more popular, it is created by the Go
authors, and it's simpler/smaller. But I'm fine sticking with afero.


Reply to this email directly or view it on GitHub
#1460 (comment).

@sqs
Copy link
Author

sqs commented Sep 26, 2015

Hey Steve, there are still some places that use filepath.Walk and filepath.EvalSymlinks, which use the OS FS, not afero, even if you change those hugofs vars. My sqs/vfs branch changes those to use afero.

Ignore what I said about VFS--that just confused things. Sticking with afero is great. But the issue in the paragraph above still remains.

@spf13
Copy link
Contributor

spf13 commented Sep 26, 2015

That's great. I think we should merge it in. It would be great to have Hugo
run in memory only (RO) when using the server mode. I actually think it
should be the default.

Steve Francia
http://stevefrancia.com
http://spf13.com
http://twitter.com/spf13

On Sat, Sep 26, 2015 at 2:54 PM, Quinn Slack notifications@github.com
wrote:

Hey Steve, there are still some places that use filepath.Walk and
filepath.EvalSymlinks, which use the OS FS, not afero, even if you change
those hugofs vars. My sqs/vfs branch changes those to use afero.

Ignore what I said about VFS--that just confused things. Sticking with
afero is great. But the issue in the paragraph above still remains.


Reply to this email directly or view it on GitHub
#1460 (comment).

@bep
Copy link
Member

bep commented Sep 26, 2015

Yea, well, this is obviously a work in progress and not mergeable as-is. (I see some breaking changes (the large portions of out-commented code)).

My motivation with all of this is to ease integration testing without needing the file system. So a solid pull request is welcomed, a one that does not add any trouble or work.

A comment on the code: I think the WalkableAferoFS belong in the afero project.

@spf13
Copy link
Contributor

spf13 commented Sep 26, 2015

I should have been more clear. I meant I think this direction is right and
when it's ready we should merge it in. Agree with @bep on WalkableAferoFS
being in afero. Excited for all the great functionality and hopefully
performance this will bring.

Steve Francia
http://stevefrancia.com
http://spf13.com
http://twitter.com/spf13

On Sat, Sep 26, 2015 at 4:42 PM, Bjørn Erik Pedersen <
notifications@github.com> wrote:

Yea, well, this is obviously a work in progress and not mergeable as-is. I
see some breaking changes (the large portions of out-commented code).

My motivation with all of this is to ease integration testing without
needing the file system. So a solid pull request is welcomed, a one that
does not add any trouble or work.

A comment on the code: I think the WalkableAferoFS belong in the afero
project.


Reply to this email directly or view it on GitHub
#1460 (comment).

@bep bep added the Enhancement label Oct 7, 2015
@bep
Copy link
Member

bep commented Nov 6, 2015

So, @sqs -- any update/progress on this?

@sqs
Copy link
Author

sqs commented Nov 8, 2015

Not yet, sorry.

@bep
Copy link
Member

bep commented Jun 11, 2016

@sqs I am closing this as stale if is OK by you?

@sqs
Copy link
Author

sqs commented Jun 11, 2016

👍 Sounds good.

On Jun 10, 2016, at 17:52, Bjørn Erik Pedersen notifications@github.com wrote:

@sqs I am closing this as stale if is OK by you?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@bep bep closed this as completed Jun 11, 2016
@ghost
Copy link

ghost commented Aug 11, 2018

@sqs do you still have the wip code available? I just found some additional motivation and could use a springboard. Product version not of interest.

@sqs
Copy link
Author

sqs commented Aug 11, 2018

@JHabdas Sorry, I must have deleted it. I can’t find it in any local clones or on GitHub.

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants