fix: use shared .ftl folder for stubs #1843

wesbillman · 2024-06-20T23:59:57Z

alecthomas

I imagine we don't really need tests, because if this wasn't working, all of our integration tests would fail.

alecthomas · 2024-06-21T00:11:49Z

.gitignore

 **/testdata/**/_ftl
 **/examples/**/_ftl
+**/testdata/**/.ftl
+**/examples/**/.ftl
+**/_ftl


Remove these, they were deliberately not present because they exclude the same name in templates.

I think I added the equivalent of what we had before with _ftl but maybe I'm missing something. I reordered these a bit to group them as well.

**/_ftl and **/.ftl, shouldn't be there. It's the reason why your new template dir is missing. They need to be removed.

Edit: sorry, I removed **/_ftl a while back but it looks like someone added it back in

But let's remove them now!

alecthomas · 2024-06-21T03:01:37Z

buildengine/engine.go

+
+		meta, ok := e.moduleMetas.Load(module)
+		if !ok {
+			return fmt.Errorf("Module %q not found", module)


nit: errors shouldn't be upper cased

alecthomas · 2024-06-21T03:03:52Z

buildengine/engine_test.go

@@ -16,7 +16,7 @@ func TestEngine(t *testing.T) {
 		t.SkipNow()
 	}
 	ctx := log.ContextWithNewDefaultLogger(context.Background())
-	engine, err := buildengine.New(ctx, nil, []string{"testdata/alpha", "testdata/other", "testdata/another"})
+	engine, err := buildengine.New(ctx, nil, t.TempDir(), []string{"testdata/alpha", "testdata/other", "testdata/another"})


Just a heads up that t.TempDir() will create a new tempdir each time it's called. Just make sure that's what you want.

I had to look that up while I was working on this :)

alecthomas · 2024-06-21T03:04:58Z

cmd/ftl/cmd_new.go

+	Replace   map[string]string `short:"r" help:"Replace a module import path with a local path in the initialised FTL module." placeholder:"OLD=NEW,..." env:"FTL_INIT_GO_REPLACE"`
+	Dir       string            `arg:"" help:"Directory to initialize the module in."`
+	Name      string            `arg:"" help:"Name of the FTL module to create underneath the base directory."`
+	GoVersion string


I added this one so that it could be used in the scaffolding template since that uses newGoCmd directly as it's context. Would it be better to create a new context and add what I need there for the template instead of using the newGoCmd for template context?

Yes, if you need it in the context but it's not a flag, it shouldn't be in the flag struct.

alecthomas · 2024-06-21T03:06:02Z

go-runtime/compile/build.go

@@ -87,14 +93,15 @@ func (b ExternalModuleContext) NonMainModules() []*schema.Module {
 	return modules
 }

-const buildDirName = "_ftl"
+const buildDirName = ".ftl"
+const DefaultGoModVersion = "1.21"


There should never be a need for something like this, we should always pull the Go version from the module's go.mod file.

This was something I wanted to chat about. With this new shared setup, I'm not sure which version to use here. Given that we might be generating shared modules for multiple modules. Or in the initial case we're generating shared modules for just builtins so I don't have a go version to get from a module. In the other cases we're very likely generating a single shared .ftl/go/modules for a multiple modules, so I wasn't sure which go version to use there either.

The big one is the builtins though. That's what led to me adding this here, but it felt dirty for sure. I also added this central one so we could share it with the cmd_new.go go code that was using a hardcoded version in it's template.

Ah good question. In that case we should use the highest version.

alecthomas · 2024-06-21T03:06:53Z

cmd/ftl/cmd_new.go

@@ -58,6 +60,8 @@ func (i newGoCmd) Run(ctx context.Context) error {

 	logger := log.FromContext(ctx)
 	logger.Debugf("Creating FTL Go module %q in %s", name, path)
+
+	i.GoVersion = compile.DefaultGoModVersion


This should never be needed, either pull it from the go.mod file or if that's really not possible, from the go tool itself.

Yeah maybe we should use the go tool for this one too. Before this change the version was hard coded in the template.

Ah yes this is for ftl new - in that case it should definitely use the Go tool.

alecthomas · 2024-06-21T03:16:15Z

go-runtime/compile/devel.go

@@ -8,9 +8,14 @@ import (
 	"github.com/TBD54566975/ftl/internal"
 )

+func mainWorkTemplateFiles() *zip.Reader {
+	return internal.ZipRelativeToCaller("main-work-template")


I don't see these files?

ahh maybe a .gitignore issue. I'll fix. Thanks!

alecthomas · 2024-06-21T03:17:54Z

go-runtime/scaffolding/{{ .Name | camel | lower }}/go.mod.tmpl

@@ -1,9 +1,9 @@
 module ftl/{{ .Name }}

-go 1.21


OMG I wonder if this is the source of all those bugs we've seen ...

alecthomas · 2024-06-21T21:33:13Z

.gitignore

 buildengine/.gitignore
 go.work*
 junit*.xml
 /readme-tests
-**/_ftl


This one shouldn't have been there.

alecthomas · 2024-06-21T21:34:36Z

.gitignore

 **/testdata/**/_ftl
 **/examples/**/_ftl
+**/testdata/**/.ftl
+**/examples/**/.ftl
+**/_ftl


But let's remove them now!

alecthomas · 2024-06-29T19:48:56Z

Let's not forget about this, looking forward to it!

alecthomas

Absolutely mammoth effort, thanks Wes :)

This reverts commit 0a69db1.

This reverts commit 0a69db1. Due to #2001

Fixes #1662 Fixes #1827

ftl-robot mentioned this pull request Jun 21, 2024

Dashboard #728

Open

alecthomas approved these changes Jun 21, 2024

View reviewed changes

wesbillman force-pushed the generate-shared-stubs branch from 80a8da6 to b6a7150 Compare June 21, 2024 18:22

alecthomas reviewed Jun 22, 2024

View reviewed changes

wesbillman force-pushed the generate-shared-stubs branch 6 times, most recently from 73fbaa0 to b975a47 Compare July 2, 2024 19:14

wesbillman marked this pull request as ready for review July 2, 2024 19:42

wesbillman requested review from a team and worstell and removed request for a team July 2, 2024 19:42

wesbillman added 6 commits July 3, 2024 09:26

fix: use shared .ftl folder for stubs

00744b9

fix: update based on pr comments

1fc528f

fix: gather schemas and go mod replacements

86eb564

fix: project config file copy over

d886d34

fix: generate replace directives and copy ftl-project to temp folder

df7b032

fix: project file path in test harness

3921263

wesbillman force-pushed the generate-shared-stubs branch from b975a47 to 9ba22ba Compare July 3, 2024 21:08

fix: build self contained external modules

caf6be9

wesbillman force-pushed the generate-shared-stubs branch from 9ba22ba to caf6be9 Compare July 3, 2024 21:16

alecthomas approved these changes Jul 4, 2024

View reviewed changes

alecthomas merged commit 0a69db1 into main Jul 4, 2024
45 checks passed

alecthomas deleted the generate-shared-stubs branch July 4, 2024 10:03

matt2e added a commit that referenced this pull request Jul 8, 2024

Revert "fix: use shared .ftl folder for stubs (#1843)"

6dad972

This reverts commit 0a69db1.

matt2e added a commit that referenced this pull request Jul 8, 2024

revert: "fix: use shared .ftl folder for stubs (#1843)" (#2003)

37423b4

This reverts commit 0a69db1. Due to #2001

wesbillman added a commit that referenced this pull request Jul 8, 2024

fix: use shared .ftl folder for stubs (#1843)

04ec858

Fixes #1662 Fixes #1827

wesbillman added a commit that referenced this pull request Jul 8, 2024

fix: use shared .ftl folder for stubs (#1843) (#2005)

5a303ee

Fixes #1662 Fixes #1827

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use shared .ftl folder for stubs #1843

fix: use shared .ftl folder for stubs #1843

wesbillman commented Jun 20, 2024 •

edited

Loading

alecthomas left a comment

alecthomas Jun 21, 2024

wesbillman Jun 21, 2024

alecthomas Jun 21, 2024 •

edited

Loading

alecthomas Jun 21, 2024

alecthomas Jun 21, 2024

alecthomas Jun 21, 2024

wesbillman Jun 21, 2024

alecthomas Jun 21, 2024

wesbillman Jun 21, 2024

alecthomas Jun 21, 2024

alecthomas Jun 21, 2024

wesbillman Jun 21, 2024

alecthomas Jun 21, 2024

alecthomas Jun 21, 2024

wesbillman Jun 21, 2024

alecthomas Jun 21, 2024

alecthomas Jun 21, 2024

wesbillman Jun 21, 2024

alecthomas Jun 21, 2024

alecthomas Jun 21, 2024

alecthomas Jun 21, 2024

alecthomas commented Jun 29, 2024

alecthomas left a comment

fix: use shared .ftl folder for stubs #1843

fix: use shared .ftl folder for stubs #1843

Conversation

wesbillman commented Jun 20, 2024 • edited Loading

alecthomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecthomas Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecthomas commented Jun 29, 2024

alecthomas left a comment

Choose a reason for hiding this comment

wesbillman commented Jun 20, 2024 •

edited

Loading

alecthomas Jun 21, 2024 •

edited

Loading