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

WIP: Generate separate data and common files. #25

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dmitshur
Copy link
Member

@dmitshur dmitshur commented Dec 23, 2016

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.

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
Copy link
Member Author

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
Copy link

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

@VojtechVitek
Copy link

VojtechVitek commented Feb 10, 2017

@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
Copy link

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
Copy link

ncruces commented Feb 1, 2019

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
Copy link

ncruces commented Feb 1, 2019

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

@mklimuk
Copy link

mklimuk commented Mar 10, 2019

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
Copy link

koliyo commented Apr 4, 2019

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

@dmitshur
Copy link
Member Author

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
Copy link

bbkane commented Jan 21, 2020

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

@ncruces
Copy link

ncruces commented Jan 21, 2020

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
Copy link

cryptix commented Feb 5, 2021

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
Copy link

bbkane commented Feb 5, 2021 via email

@dmitshur
Copy link
Member Author

dmitshur commented Feb 5, 2021

@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
Copy link

cryptix commented Feb 7, 2021

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

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

Successfully merging this pull request may close these issues.

Support generating multiple variables (within same package).
9 participants