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

Auto infer module when bud create outside $GOPATH #242

Merged
merged 4 commits into from
Aug 10, 2022

Conversation

012e
Copy link
Contributor

@012e 012e commented Aug 8, 2022

Implements #240

@012e
Copy link
Contributor Author

012e commented Aug 8, 2022

@matthewmueller should I rename my branch?

bud/Makefile

Line 253 in e23cd48

GOPRIVATE=github.com/livebud/bud go install github.com/livebud/bud@$(BRANCH_NAME)

For example,
bud create --module=github.com/my/app %s
`, c.Dir))
module.Name = "changeme"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should make it change.me because changeme isn't a valid module name. This doesn't appear to have any ramifications right now, but I don't see a big deal with generating a valid name. See below for an example:

package main

import (
	"fmt"
	"os"

	"golang.org/x/mod/module"
)

func main() {
	if len(os.Args) <= 1 {
		fmt.Println("no args")
		return
	}
	modulePath := os.Args[1]
	err := module.CheckPath(modulePath)
	if err != nil {
		fmt.Printf("invalid path %s: %s\n", modulePath, err)
		os.Exit(1)
	}
	fmt.Printf("valid path %s\n", modulePath)
}

is.Equal(result.Stdout(), "")
is.Equal(result.Stderr(), "")
is.NoErr(td.NotExists(".gitignore"))
is.Equal(fileFirstLine(dir+"/go.mod"), "module changeme\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

filepath.Join(dir, "go.mod")

@@ -37,6 +50,7 @@ func TestCreateOutsideGoPathModulePath(t *testing.T) {
is.NoErr(err)
is.Equal(result.Stdout(), "")
is.Equal(result.Stderr(), "")
is.Equal(fileFirstLine(dir+"/go.mod"), "module github.com/my/app\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

@matthewmueller
Copy link
Contributor

matthewmueller commented Aug 9, 2022

Thanks for working on this @012e! Just reviewed.

@matthewmueller should I rename my branch?

This looks like a bug in the recent Makefile E2E test not accounting for branches outside of livebud/bud.

I think the problem is that auto-infer-module branch doesn't exist on livebud/bud. It only exists on your branch.

Could you try changing the Makefile and seeing if you can get make e2e working on your branch? It seems like we could use the GITHUB_REPOSITORY environment variable in Github Actions: https://docs.github.com/en/actions/learn-github-actions/environment-variables

So maybe something like the following:

BRANCH_NAME ?= $(or $(GITHUB_HEAD_REF),main)
+ GITHUB_REPO ?= $(or $(GITHUB_REPOSITORY),livebud/bud)
GOPATH := $(shell go env GOPATH)


e2e: e2e.bud.build


e2e.bud.build:
-	GOPRIVATE=github.com/livebud/bud go install github.com/livebud/bud@$(BRANCH_NAME)
+	GOPRIVATE=github.com/$(GITHUB_REPO) go install github.com/$(GITHUB_REPO)@$(BRANCH_NAME)
	git clone https://github.com/livebud/welcome
	( cd welcome && \
		npm install && \
		go mod tidy && \
-		GOPRIVATE=github.com/livebud/bud go get github.com/livebud/bud@$(BRANCH_NAME) \
+               GOPRIVATE=github.com/$(GITHUB_REPO) go get github.com/$(GITHUB_REPO)@$(BRANCH_NAME) \
	)
	$(GOPATH)/bin/bud -C welcome build
	./welcome/bud/app -h

@012e
Copy link
Contributor Author

012e commented Aug 9, 2022

BRANCH_NAME ?= $(or $(GITHUB_HEAD_REF),main)
+ GITHUB_REPO ?= $(or $(GITHUB_REPOSITORY),livebud/bud)
GOPATH := $(shell go env GOPATH)


e2e: e2e.bud.build


e2e.bud.build:
-	GOPRIVATE=github.com/livebud/bud go install github.com/livebud/bud@$(BRANCH_NAME)
+	GOPRIVATE=github.com/$(GITHUB_REPO) go install github.com/$(GITHUB_REPO)@$(BRANCH_NAME)
	git clone https://github.com/livebud/welcome
	( cd welcome && \
		npm install && \
		go mod tidy && \
-		GOPRIVATE=github.com/livebud/bud go get github.com/livebud/bud@$(BRANCH_NAME) \
+               GOPRIVATE=github.com/$(GITHUB_REPO) go get github.com/$(GITHUB_REPO)@$(BRANCH_NAME) \
	)
	$(GOPATH)/bin/bud -C welcome build
	./welcome/bud/app -h

After a while researching, I found we can't go install a fork. Should we just remove the e2e test for now and create a new issue?

@matthewmueller
Copy link
Contributor

matthewmueller commented Aug 10, 2022

Thanks @012e for the research! I just updated the e2e target to hopefully address this. Re-running tests.

@matthewmueller matthewmueller merged commit ce4f4bd into livebud:main Aug 10, 2022
@matthewmueller
Copy link
Contributor

Cheers, thanks Huy!

@012e 012e deleted the auto-infer-module branch August 10, 2022 07:49
matthewmueller pushed a commit that referenced this pull request Aug 24, 2022
…ATH (#242)

* auto infer module outside GOPATH
* fix create test
* default module name to `change.me`
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.

None yet

2 participants