gobuffalo / genny

A framework for writing modular generators
MIT License
65 stars 14 forks source link

temporary files under the /tmp #18

Closed sio4 closed 5 years ago

sio4 commented 5 years ago

Hi, I found the code below generates so many files under the /tmp but they are not removed after the build (in fact, buffalo dev). Also, I saw a comment something like "virtual file" and "virtual disk", and comment for Disk.Find() said, it tries to find a file from virtual disk but if the file does not exist, then check for a real filesystem. Are they the physical files as a backup for this purpose? (or just remained old code?)

https://github.com/gobuffalo/genny/blob/849d2c9534eab3ab52e2a8e5992cb6521fd7505c/movinglater/gotools/parsed_file.go#L66-L75

markbates commented 5 years ago

Those files should get cleaned up; agreed. We have to pull that trick with the temp directory because Go’s AST parsers only work on physical files and genny, most files live in memory until they’re finally generated to disk. So we have store the in memory files in temp to parse them so we can manipulate the AST. it’s a hack; but one that works.

The disk stuff you mention is completely different. That’s one of the things that makes genny “work” when you ask for a file form the “disk” if it already has it memory it returns the in memory version otherwise it loads it from disk into memory.

They don’t have anything to do with each other, but I can understand the confusion. :)

A PR to clean up those temp files would be great. :) On Dec 5, 2018, 5:09 AM -0500, Yonghwan SO notifications@github.com, wrote:

Hi, I found the code below generates so many files under the /tmp but they are not removed after the build (in fact, buffalo dev). Also, I saw a comment something like "virtual file" and "virtual disk", and comment for Disk.Find() said, it tries to find a file from virtual disk but if the file does not exist, then check for a real filesystem. Are they the physical files as a backup for this purpose? (or just remained old code?) https://github.com/gobuffalo/genny/blob/849d2c9534eab3ab52e2a8e5992cb6521fd7505c/movinglater/gotools/parsed_file.go#L66-L75 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

sio4 commented 5 years ago

Oh, OK. You means those files should be there while processing and should be cleaned when the processing is done. Right?

I looked into genny today as a very first time and I want to write a PR since my root partition was almost full! :-) I will check the code and flow. Anyway, if there are some hints, it will help me a lot to speed up!

Thanks.

-- ...and in the end, the love you take is equal to the love make

2018년 12월 5일 (수) 20:29에 Mark Bates notifications@github.com님이 작성:

Those files should get cleaned up; agreed. We have to pull that trick with the temp directory because Go’s AST parsers only work on physical files and genny, most files live in memory until they’re finally generated to disk. So we have store the in memory files in temp to parse them so we can manipulate the AST. it’s a hack; but one that works.

The disk stuff you mention is completely different. That’s one of the things that makes genny “work” when you ask for a file form the “disk” if it already has it memory it returns the in memory version otherwise it loads it from disk into memory.

They don’t have anything to do with each other, but I can understand the confusion. :)

A PR to clean up those temp files would be great. :) On Dec 5, 2018, 5:09 AM -0500, Yonghwan SO notifications@github.com, wrote:

Hi, I found the code below generates so many files under the /tmp but they are not removed after the build (in fact, buffalo dev). Also, I saw a comment something like "virtual file" and "virtual disk", and comment for Disk.Find() said, it tries to find a file from virtual disk but if the file does not exist, then check for a real filesystem. Are they the physical files as a backup for this purpose? (or just remained old code?)

https://github.com/gobuffalo/genny/blob/849d2c9534eab3ab52e2a8e5992cb6521fd7505c/movinglater/gotools/parsed_file.go#L66-L75 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/gobuffalo/genny/issues/18#issuecomment-444453623, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEPHuhK8sPmTYE6YAyLos7A8Uq12O_Rks5u164WgaJpZM4ZCZno .

markbates commented 5 years ago

Yeah, as soon as they’re done bring process they can be deleted.

I need to write up some docs; I know. :(

Basically genny is about the Runner and Generator types. Everything else is basically some hook into those. A On Dec 5, 2018, 6:36 AM -0500, Yonghwan SO notifications@github.com, wrote:

Oh, OK. You means those files should be there while processing and should be cleaned when the processing is done. Right?

I looked into genny today as a very first time and I want to write a PR since my root partition was almost full! :-) I will check the code and flow. Anyway, if there are some hints, it will help me a lot to speed up!

Thanks.

-- ...and in the end, the love you take is equal to the love make

2018년 12월 5일 (수) 20:29에 Mark Bates notifications@github.com님이 작성:

Those files should get cleaned up; agreed. We have to pull that trick with the temp directory because Go’s AST parsers only work on physical files and genny, most files live in memory until they’re finally generated to disk. So we have store the in memory files in temp to parse them so we can manipulate the AST. it’s a hack; but one that works.

The disk stuff you mention is completely different. That’s one of the things that makes genny “work” when you ask for a file form the “disk” if it already has it memory it returns the in memory version otherwise it loads it from disk into memory.

They don’t have anything to do with each other, but I can understand the confusion. :)

A PR to clean up those temp files would be great. :) On Dec 5, 2018, 5:09 AM -0500, Yonghwan SO notifications@github.com, wrote:

Hi, I found the code below generates so many files under the /tmp but they are not removed after the build (in fact, buffalo dev). Also, I saw a comment something like "virtual file" and "virtual disk", and comment for Disk.Find() said, it tries to find a file from virtual disk but if the file does not exist, then check for a real filesystem. Are they the physical files as a backup for this purpose? (or just remained old code?)

https://github.com/gobuffalo/genny/blob/849d2c9534eab3ab52e2a8e5992cb6521fd7505c/movinglater/gotools/parsed_file.go#L66-L75 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/gobuffalo/genny/issues/18#issuecomment-444453623, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEPHuhK8sPmTYE6YAyLos7A8Uq12O_Rks5u164WgaJpZM4ZCZno .

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

sio4 commented 5 years ago

Hi @markbates,

It looks like the temporary file is used within the function gotools.ParseFileMode() so I can simply add os.Remove(gf.Name()) after parse.ParseFile() (or defer it just after beforeParse()).

Anyway, although I could not dig into all sources of go parser (and related packages), it looks like parser.ParseFile() can work without the physical file. As I look into the ParseFile(), it takes src as the content of the file and FileSet.AddFile() called by parser.init() also takes len(src) as a size of the file. Can you check on? I think we can safely remove this part from the function.

sio4 commented 5 years ago

All test cases are passed when I tested with the code below, and after rebuild genny, buffalo, and buffalo-goth, it looks like buffalo new and buffalo generate goth ... works fine.

func ParseFileMode(gf genny.File, mode parser.Mode) (ParsedFile, error) {
    pf := ParsedFile{
        FileSet: token.NewFileSet(),
        File:    gf,
    }

    src := gf.String()
    f, err := parser.ParseFile(pf.FileSet, gf.Name(), src, mode)
    if err != nil && errors.Cause(err) != io.EOF {
        return pf, errors.WithStack(err)
    }
    pf.Ast = f

    pf.Lines = strings.Split(src, "\n")
    return pf, nil
}
markbates commented 5 years ago

PR? On Dec 6, 2018, 2:11 AM -0500, Yonghwan SO notifications@github.com, wrote:

All test cases are passed when I tested with the code below, and after rebuild genny, buffalo, and buffalo-goth, it looks like buffalo new and buffalo generate goth ... works fine. func ParseFileMode(gf genny.File, mode parser.Mode) (ParsedFile, error) { pf := ParsedFile{ FileSet: token.NewFileSet(), File: gf, }

   src := gf.String()
   f, err := parser.ParseFile(pf.FileSet, gf.Name(), src, mode)
   if err != nil && errors.Cause(err) != io.EOF {
           return pf, errors.WithStack(err)
   }
   pf.Ast = f

   pf.Lines = strings.Split(src, "\n")
   return pf, nil

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