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

Changes wrt json5 fork #1498

Closed
utkarsh2102 opened this issue Feb 12, 2020 · 5 comments
Closed

Changes wrt json5 fork #1498

utkarsh2102 opened this issue Feb 12, 2020 · 5 comments

Comments

@utkarsh2102
Copy link
Contributor

Hi Zyedidia,

Whilst updating the Debian package of micro to 2.0.0, the build failed with json5.
I saw that you started using your own fork of json5 with only a single commit on top. Is it really necessary to do that?

We were using golang-github-flynn-json5 in Debian but now we'll have to package your work - which causes extra work?
Is it just possible to use the original fork?

Alternatively, I could just apply the "Remove test dependencies" patch on top of the golang-github-flynn-json5 module that we have in Debian. Would that work?

@zyedidia
Copy link
Owner

zyedidia commented Feb 12, 2020

Ah sorry about this. The flynn/json5 repo depended on otto (a javascript interpreter written in Go) for its tests, and this caused micro to depend on otto and a number of other large projects. To avoid having go mod download a huge number of unnecessary dependencies, I forked json5 and removed the uses of otto in the tests.

It should be possible to use the code from flynn/json5 (my fork did not change any of the source code), but since micro's source code refers to the zyedidia/json5 package it will have to be placed at the correct location. How does your system currently build micro using the json5 package? Does it unpack the package to a certain location in the $GOPATH? If for example, it puts the source at $GOPATH/src/github.com/flynn/json5, you could instead just put it at $GOPATH/src/github.com/zyedidia/json5, and I think everything would work.

@utkarsh2102
Copy link
Contributor Author

It fails to build like this:

dh_auto_build -- -buildmode=pie \
-ldflags="-extldflags -Wl,-z,now -s -w -X main.Version=2.0.0-1 -X main.CommitHash= -X 'main.CompileDate=' "
	cd obj-x86_64-linux-gnu && go install -trimpath -v -p 8 -buildmode=pie "-ldflags=-extldflags -Wl,-z,now -s -w -X main.Version=2.0.0-1 -X main.CommitHash= -X 'main.CompileDate=' " github.com/zyedidia/micro/cmd/micro
src/github.com/zyedidia/micro/internal/action/bindings.go:12:2: cannot find package "github.com/zyedidia/json5" in any of:
	/usr/lib/go-1.13/src/github.com/zyedidia/json5 (from $GOROOT)
	/home/utkarsh/debian/updates/go-team/micro/micro/obj-x86_64-linux-gnu/src/github.com/zyedidia/json5 (from $GOPATH)
dh_auto_build: error: cd obj-x86_64-linux-gnu && go install -trimpath -v -p 8 -buildmode=pie "-ldflags=-extldflags -Wl,-z,now -s -w -X main.Version=2.0.0-1 -X main.CommitHash= -X 'main.CompileDate=' " github.com/zyedidia/micro/cmd/micro returned exit code 1

@utkarsh2102
Copy link
Contributor Author

However, just to see how it behaves if I replace zyedidia/json5 with flynn/json5, I am applying this patch:

--- a/go.mod
+++ b/go.mod
@@ -14,7 +14,7 @@
 	github.com/zyedidia/clipboard v0.0.0-20190823154308-241f98e9b197
 	github.com/zyedidia/glob v0.0.0-20170209203856-dd4023a66dc3
 	github.com/zyedidia/highlight v0.0.0-20170330143449-201131ce5cf5
-	github.com/zyedidia/json5 v0.0.0-20200102012142-2da050b1a98d
+	github.com/flynn/json5 v0.0.0-20200102012142-2da050b1a98d
 	github.com/zyedidia/pty v2.0.0+incompatible // indirect
 	github.com/zyedidia/tcell v1.4.3
 	github.com/zyedidia/terminal v0.0.0-20180726154117-533c623e2415
--- a/go.sum
+++ b/go.sum
@@ -44,8 +44,8 @@
 github.com/zyedidia/glob v0.0.0-20170209203856-dd4023a66dc3/go.mod h1:YKbIYP//Eln8eDgAJGI3IDvR3s4Tv9Z9TGIOumiyQ5c=
 github.com/zyedidia/highlight v0.0.0-20170330143449-201131ce5cf5 h1:Zs6mpwXvlqpF9zHl5XaN0p5V4J9XvP+WBuiuXyIgqvc=
 github.com/zyedidia/highlight v0.0.0-20170330143449-201131ce5cf5/go.mod h1:c1r+Ob9tUTPB0FKWO1+x+Hsc/zNa45WdGq7Y38Ybip0=
-github.com/zyedidia/json5 v0.0.0-20200102012142-2da050b1a98d h1:zmDMkh22zXOB7gz8jFaI4GpI7llsPgzm38/jG0UgxjE=
-github.com/zyedidia/json5 v0.0.0-20200102012142-2da050b1a98d/go.mod h1:NDJSTTYWivnza6zkRapeX2/LwhKPEMQ7bJxqgDVT78I=
+github.com/flynn/json5 v0.0.0-20200102012142-2da050b1a98d h1:zmDMkh22zXOB7gz8jFaI4GpI7llsPgzm38/jG0UgxjE=
+github.com/flynn/json5 v0.0.0-20200102012142-2da050b1a98d/go.mod h1:NDJSTTYWivnza6zkRapeX2/LwhKPEMQ7bJxqgDVT78I=
 github.com/zyedidia/poller v1.0.1 h1:Tt9S3AxAjXwWGNiC2TUdRJkQDZSzCBNVQ4xXiQ7440s=
 github.com/zyedidia/poller v1.0.1/go.mod h1:vZXJOHGDcuK08GXhF6IAY0ZFd2WcgOR5DOTp84Uk5eE=
 github.com/zyedidia/pty v2.0.0+incompatible h1:Ou5vXL6tvjst+RV8sUFISbuKDnUJPhnpygApMFGweqw=
--- a/internal/action/bindings.go
+++ b/internal/action/bindings.go
@@ -9,7 +9,7 @@
 	"strings"
 	"unicode"
 
-	"github.com/zyedidia/json5"
+	"github.com/flynn/json5"
 	"github.com/zyedidia/micro/internal/config"
 	"github.com/zyedidia/micro/internal/screen"
 	"github.com/zyedidia/tcell"
--- a/internal/config/plugin_installer.go
+++ b/internal/config/plugin_installer.go
@@ -15,7 +15,7 @@
 
 	"github.com/blang/semver"
 	lua "github.com/yuin/gopher-lua"
-	"github.com/zyedidia/json5"
+	"github.com/flynn/json5"
 	ulua "github.com/zyedidia/micro/internal/lua"
 	"github.com/zyedidia/micro/internal/util"
 )
--- a/internal/config/plugin_installer_test.go
+++ b/internal/config/plugin_installer_test.go
@@ -5,7 +5,7 @@
 
 	"github.com/blang/semver"
 
-	"github.com/zyedidia/json5"
+	"github.com/flynn/json5"
 )
 
 func TestDependencyResolving(t *testing.T) {
--- a/internal/config/settings.go
+++ b/internal/config/settings.go
@@ -11,7 +11,7 @@
 	"strings"
 
 	"github.com/zyedidia/glob"
-	"github.com/zyedidia/json5"
+	"github.com/flynn/json5"
 	"github.com/zyedidia/micro/internal/util"
 	"golang.org/x/text/encoding/htmlindex"
 )

P.S. I am aware that I should not patch go.mod and go.sum, but I am just experimenting with the builds to see how it gets affected.
And with this patch, I land up at #1499 :/

@zyedidia
Copy link
Owner

Well at least with the patch it looks like the flynn/json5 issue is resolved. I think #1499 will be harder to resolve since that is due to significant changes in the dependency. I think for that one the debian package will need to be updated.

@utkarsh2102
Copy link
Contributor Author

Yep, working on that update.
Build failure for tcell update to 1.4.3 is posted on #1499 :)

Maybe next week I'll rather package your json5 fork, too :)

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

No branches or pull requests

2 participants