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

create: replace vfs with embed #637

Merged
merged 2 commits into from
Mar 29, 2022
Merged

create: replace vfs with embed #637

merged 2 commits into from
Mar 29, 2022

Conversation

mRrvz
Copy link
Contributor

@mRrvz mRrvz commented Sep 23, 2021

Reworked by @DifferentialOrange

Go requirement was bumped to 1.18.
To solve #631 issue, we need to use Go with fixed embedding of
folders for case when only contents are files starting with dot [1].
The fix was introduced in Go 1.18.

Since Go 1.16, it is possible to use "embed" library to work with
virtual file systems. We use one for templates in cartridge create.
Before this patch, we used third party library [2]. Now it's replaced
with "embed".

To support embedding folder which only contents are file starting with
dot, we need to use Go 1.18. Moreover, file with "|" symbol in name
considered to be invalid [3] and its embedding is not supported.
So approach from #612 was reworked to use lowered app name as variable
instead of transforming it with Funcs.

Directories in vfs now have "dr-xr-xr-x" permissions instead of
"drwxr-xr-x", so we change it on template build (otherwise it would be
impossible to create subdirectories and files in directories).

Running go mod tidy regrouped go.mod contents, together with
cleaning up [2] dependency.

  1. cmd/go: add //go:embed all:<pattern> to allow . and _ matches golang/go#43854
  2. https://github.com/shurcooL/vfsgen
  3. https://cs.opensource.google/go/x/mod/+/refs/tags/v0.5.1:module/module.go;l=488

Closes #631

@mRrvz mRrvz marked this pull request as draft September 23, 2021 21:28
@mRrvz mRrvz mentioned this pull request Sep 23, 2021
@mRrvz mRrvz mentioned this pull request Nov 16, 2021
@Totktonada
Copy link
Member

Looks like a breaking change. Can you clarify, whether it discards support of some Go versions?

@DifferentialOrange DifferentialOrange marked this pull request as ready for review January 11, 2022 13:02
@DifferentialOrange DifferentialOrange marked this pull request as draft January 11, 2022 13:03
@DifferentialOrange DifferentialOrange force-pushed the embeded-fs branch 2 times, most recently from ac9ec68 to 715a416 Compare January 12, 2022 07:35
DifferentialOrange added a commit that referenced this pull request Jan 12, 2022
To solve #631 issue, we need to use Go with fixed embedding of
folders for case when only contents are files starting with dot [1].
It should be introduced in Go 1.18, but it is not released yet.

1. golang/go#43854

Part of #637
@DifferentialOrange DifferentialOrange marked this pull request as ready for review January 12, 2022 10:02
@DifferentialOrange
Copy link
Member

Maybe it should wait until Go 1.18 stable release, but I'll press review request button since it works

DifferentialOrange added a commit that referenced this pull request Jan 27, 2022
To solve #631 issue, we need to use Go with fixed embedding of
folders for case when only contents are files starting with dot [1].
It should be introduced in Go 1.18, but it is not released yet.

1. golang/go#43854

Part of #637
Copy link
Member

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no objections here, but, it seems, we should wait until Go 1.18 will be released.

@Totktonada Totktonada added the do not merge Not ready to be merged label Feb 4, 2022
@DifferentialOrange DifferentialOrange removed the do not merge Not ready to be merged label Mar 18, 2022
@DifferentialOrange
Copy link
Member

Do we include internal changes like this in CHANGELOG or should it only include changes that affect binary artifact user?

@DifferentialOrange
Copy link
Member

@LeonidVas it's relevant again

To solve #631 issue, we need to use Go with fixed embedding of
folders for case when only contents are files starting with dot [1].
The fix was introduced in Go 1.18.

1. golang/go#43854

Part of #631
Since Go 1.16, it is possible to use "embed" library to work with
virtual file systems. We use one for templates in ``cartridge create``.
Before this patch, we used third party library [1]. Now it's replaced
with "embed".

To support embedding folder which only contents are file starting with
dot, we need to use Go 1.18. Moreover, file with "|" symbol in name
considered to be invalid [2] and its embedding is not supported.
So approach from #612 was reworked to use lowered app name as variable
instead of transforming it with Funcs.

Directories in vfs now have "dr-xr-xr-x" permissions instead of
"drwxr-xr-x", so we change it on template build (otherwise it would be
impossible to create subdirectories and files in directories).

Running ``go mod tidy`` regrouped go.mod contents, together with
cleaning up [1] dependency.

1. https://github.com/shurcooL/vfsgen
2. https://cs.opensource.google/go/x/mod/+/refs/tags/v0.5.1:module/module.go;l=488

Closes #631
@LeonidVas LeonidVas merged commit 29a2cc2 into master Mar 29, 2022
@LeonidVas LeonidVas deleted the embeded-fs branch March 29, 2022 14:58
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.

Replace vfs with embed
4 participants