WIP: Generate separate data and common files.

[dmitshur]

The goal is to resolve #23, to make it possible to use vfsgen multiple times in same package.

Previously, that would cause an error because the same types would be defined in each generated file.

The approach to solve that is to generate two files:

• VFS data file
• common type definitions file

This avoids the multiple declaration issues in an efficient way.

A downside is having 2 files as generated output, instead of one.

[dmitshur]

Some open questions remaining:

1. Small question. What to name the generated common file. I went with "vfsgencommon.go", is that okay? It should always be the same name so that multiple invocations of vfsgen don't result in multiple common files (the goal is to always have just one per package).

2. Big question. It currently changes the behavior to always generate 2 files. Is that the way to go? Or is it better to make the "common" file an optional feature that user has to opt in, otherwise use previous behavior of generating one file?

   Giving user control makes for a harder to use API (they have to understand what's correctly enable the common file when needed), and complicates the implementation because there are 2 different generation paths. But if the majority of users don't need common file, it's nicer to have just 1 generated output file...

[annismckenzie]

💃 💃 💃
That'll make my build process a bit easier. I'm okay with two files.

[VojtechVitek]

@shurcooL can we make this an option in vfsgen.Options{}, ie. CommonFilename? I'm not a big fan of two output files by default (personally, I'd prefer two different packages).

vfsgen.Generate(http.Dir("templates"), vfsgen.Options{
	PackageName:    "static",
	Filename:       "templates_static.go",
	CommonFilename: "vfsgen_common.go",
	BuildTags:      "production",
	VariableName:   "TemplatesFS",
}
vfsgen.Generate(http.Dir("css"), vfsgen.Options{
	PackageName:    "static",
	Filename:       "css_files_static.go",
	CommonFilename: "vfsgen_common.go",
	BuildTags:      "production",
	VariableName:   "CssFS",
}

and by default, the common stuff would be appended to a single generated file?

[neclepsio]

I'm ok with a fixed file name for common parts and as many files as the number of vfsgen.Generate calls. I don't think it's necessary, but maybe you could make the common name configurable by package level variable.

[ncruces]

I'm sure you guys thought of this (so I'm probably misunderstanding the issue), but bear with me.

Assuming the common part stays the same (structure definitions and behavior) why isn't it put in a package in this repo and imported from however many generated files?

I mean any variability in the trailer seems to be removing some code happens not to be used (no directories, no uncompressed). Are the performance gains of bundling this worth the complexity?

I'm perfectly willing to do the legwork if any of this makes sense to you.

[ncruces]

I see in #63 you're considering this.
I'll have a go at this when I have the time.

[mklimuk]

I totally agree with @ncruces. There is no point in regenerating structures that are fixed for every instance. And an import of the generating package in a generated file feels pretty natural to me. This is the approach they took for https://github.com/golang/protobuf for instance. I will try to submit a proposal in a few days.

[koliyo]

Please complete this MR, really cumbersome to work around this limitation. Thank you for working on this! 👍

[dmitshur]

I would like to complete the general idea started here, but it will happen no earlier than a few weeks from now, and I plan to start relying on modules to implement the idea. It may make sense to make it a feature of a v2 API, and keep v1 as is. I'll post updates in #23 when I make progress.

[bbkane]

Also looking forward to this (as well as modules support). Thank you!

[ncruces]

I ended up doing my own thing because I had slightly different requirements, but this is the gist of it:

A package implementing an in memory file system: github.com/ncruces/go-fs/memfs
All code lives here, because it's static, common to all file systems.

A command that generates a source.go file which creates an instance of the in memory file system at compile/init time: https://github.com/ncruces/go-fs/tree/master/memfsgen

The generated code imports the above package, declares a single variable, and has an init function initializing the variable. There's no namespace pollution, you can put the generated code on any package of your choice. You can have multiple filesystems in the same package, or in different packages, they'll all share code.

The only slightly ugly bit is that the file system package exports a slightly uglier than I wished function that is intended to be used by static generators, and which skips most validation for performance. Using it incorrectly can lead to panics, and seemingly unrelated errors.

[cryptix]

Anything one can do to help this move forward?

Here is my very nasty workaround that I just added after the generator code:

	// nasty hack to strip duplicate type information
	err = exec.Command("sed", "-i", "/^type vfsgen۰FS/,$d", "assets_vfsdata.go").Run()
	if err != nil {
		log.Fatalln(err)
	}

	// clean up the unused imports
	err = exec.Command("goimports", "-w", ".").Run()
	if err != nil {
		log.Fatalln(err)
	}

[bbkane]

It seems like most people are using separate packages for separate embedded filesystems. Go 1.16 is supposed to release this month and will have file embedding, which does a lot of what vfsgen does.

…

On Fri, Feb 5, 2021, 00:17 Henry ***@***.***> wrote: Anything one can do to help this move forward? Here is my very nasty workaround that I just added after the generator code: // nasty hack to strip duplicate type information err = exec.Command("sed", "-i", "/^type vfsgen۰FS/,$d", "assets_vfsdata.go").Run() if err != nil { log.Fatalln(err) } // clean up the unused imports err = exec.Command("goimports", "-w", ".").Run() if err != nil { log.Fatalln(err) } — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#25 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABOMUPJNB3NCUB6WOKWOQETS5OSS7ANCNFSM4C2SDLWQ> .

[dmitshur]

@cryptix I expect major future development changes (such as this one) for this project will need to be rebased to consider the Go 1.16's //go:embed and io/fs.FS. So this is unlikely to land in vfsgen v1. I think using your workaround for the time being is a good solution. Edit: Filed a tracking issue #91.

[cryptix]

Oh wow.. I totally missed the embed feature. Thanks.