Skip to content

proposal: Replace in-house implementation of gopherjs build tool with augmented forked cmd/go build tool. #388

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.

Sign up for GitHub

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

Jump to bottom
Closed
dmitshur opened this issue Jan 30, 2016 · 13 comments
Closed

proposal: Replace in-house implementation of gopherjs build tool with augmented forked cmd/go build tool. #388

dmitshur opened this issue Jan 30, 2016 · 13 comments
Labels
gopherjs-tool Related to the gopherjs tool or its build system (but not the compiler itself). Proposal

Comments

@dmitshur
Copy link
Member

The GopherJS project has developed the gopherjs tool. Its CLI mimics a lot of the commands that go tool (import path cmd/go, in GOROOT) offers, in addition to one new command serve (see #268). However, it's missing some features of the real cmd/go (see issues labeled cmd/go for some examples).

Instead of continuing to develop and maintain the gopherjs tool ourselves, I propose we replace the custom implementation of gopherjs with a modified fork of the real cmd/go that has 2 changes cherry-picked on top. All existing functionality would be preserved, gaining missing features, fixing bugs, and reducing overall maintenance cost. The only functionality that the current gopherjs tool offers but cmd/go doesn't is the serve command, and we will preserve it in some way (details to be determined).

Plan

At a high level, my proposed plan is:

  1. Wait for Go 1.6 release, or combine this plan with adding suppport for Go 1.6.
  2. Create a fork of cmd/go.
  3. Create a change that can be cherry-picked (or rebased) on top of latest upstream cmd/go that makes the command go-gettable.
    • This is a small change.
  4. Create a change that can be cherry-picked (or rebased) on top that adds support for -compiler=gopherjs value.
    • This is a larger change, but it can be done cleanly and will result in significant simplification in the work that GopherJS needs to do, stripping away a lot of its responsibility as a build tool, leaving only the compilation and linking to be done.
  5. Continue to make the augmented cmd/go available and maintain it by simply taking the latest upstream code and rebasing the 2 cherry-picked changes on top.
    • When the plan is complete and the changes are reviewed and made clean, and everything proves to be stable, we can attempt to make a CL to merge the -compiler=gopherjs support addition upstream into core Go repository, and learn if there are any blockers from making that possible. If not accepted, we can continue to maintain the forked tool ourselves with low maintenance overhead.

The end result will be a fully capable cmd/go-like command that will support compiling with GOARCH=js set. It will be able to go run and go test by setting -exec=node. It will resolve all these issues and simplify original code that GopherJS project has to maintain.

Research and viability

I have explored this idea recently by attempting to do this during a 48-hour Gopher Gala 2016 hackathon. The project, which was mostly me learning about how cmd/go is structured and works, can be seen here:

https://github.com/gophergala2016/cmd-go-js#readme

It partially implements what I have described above, but not completely. Due to time constraints, I did not attempt to make changes to gopherjs binary and simply used it as it exists today. This allowed me to implement go build and go run, but not the other commands.

To have all other features work as expected, the entire toolchain interface needs to be implemented correctly.

You can read the README of the linked project for additional details and observations.

Mockup

Here's a mockup demonstrating how the new build tool for GopherJS would look like:

One possible simplification might be that setting -compiler=gopherjs explicitly won't be needed when GOARCH=js is already set. That would allow typed commands to be shorter.

Summary

By implementing this plan, the end result is expected to:

  • Continue to provide all existing functionality, while resolving many bugs and implementing missing functionality of the cmd/go build tool.
  • Remove most of the current "build tool" code, allowing GopherJS to focus on being a pure Go compiler and linker for JavaScript architecture.
  • Reduce the total maintenance cost since it's expected the effort to rebase the 2 clean changes on top of cmd/go will be much smaller than to keep developing and maintaining gopherjs tool ourselves.
  • A real go tool should be more familiar and friendly to new people, since most Go developers are very familiar with the cmd/go tool and its flags.
    • Compiling with GopherJS can be seen just like cross-compiling to another OS/architecture, which is a common and well understood concept.

I've discussed this plan with @neelance and agreed it sounds favorable. What's next is to attempt to execute it and ensure no major blocking issues come up.
@dmitshur dmitshur changed the title proposal: proposal: Replace in-house implementation of gopherjs build tool with augmented forked cmd/go build tool. Jan 30, 2016
@dmitshur dmitshur added Proposal go1.6 gopherjs-tool Related to the gopherjs tool or its build system (but not the compiler itself). labels Jan 30, 2016
@dominikh
Copy link
Member

I only have one concern: Should our forked command still be called "go"? That is, do we expect people who work with both Go and GopherJS to use our fork exclusively? That might encounter some resistance. Maybe it's a good idea to name the tool something else (gopherjs?), and make GOARCH=js and compiler=gopherjs the defaults for that tool?

Apart from that, 👍 to the idea of reusing the go tool.

@dmitshur
Copy link
Member Author

That's a valid concern. I think we should begin by keeping the external CLI the same - the command would still be named gopherjs, and its import path/installation may still be go get -u github.com/gopherjs/gopherjs (if we can do that cleanly, and if it's favorable to do that).

This proposal is primarily about changing the internal implementation details of the current gopherjs tool.

That said, it definitely hints of the possibility and paves the way to being able to use the forked version as your primarily go tool, and, of course, if the change is accepted upstream, they become one and the same. But all that is best to leave to a future proposal, I think.

@ghost
Copy link

ghost commented Feb 29, 2016

I support this, but agree that it should be a different name, so that developers can still run the safe and steady go tool.
One stabilised and trusted, then it could be merged into mainline go tool.

@dmitshur
Copy link
Member Author

That's the plan. :) It will be called gopherjs initially, replacing the current gopherjs binary, and behave in the same way. Only the internal code will change.

One stabilised and trusted, then it could be merged into mainline go tool.

Yep. That will require us to be confident in it, but also for the Go team to be able to accept it. It may or may not happen for some time, we'll see. First step is to create the new gopherjs binary implementation successfully. I'll be looking at doing that, but it may take some time.

@ghost
Copy link

ghost commented Feb 29, 2016

ok i did not understand the original intention then: )
go for it ! - really looking forward to trying this out .

@dmitshur dmitshur mentioned this issue Mar 22, 2016
Open
@dmitshur dmitshur mentioned this issue Apr 24, 2016
Closed
dmitshur referenced this issue Apr 26, 2016
@neelance
Improved tool CLI.
da075de
@bep
Copy link

bep commented May 7, 2016

I agree totally, but would add one item:

I have been writing lots of tests in a GopherJS project lateley. I work in LiteIDE. In that Editor I can hit cmd+T to fun all the tests in the current package. I can "rewire" the command on a per session basis, but it would be nice if:

go test <some-package>

Could look at the js build tag and decide which compiler to use, so, from an editor's perspective, I could hit cmd+T from any Go or GopherJS package and it just worked.

This would maybe even be possible with some kind of wrapper/preprocessor?

@dmitshur
Copy link
Member Author

dmitshur commented May 7, 2016 •
edited
Loading

I think the following optimization might be possible.

When you specify GOARCH=js, it can pick gopherjs compiler automatically. So instead of having to type:

GOARCH=js go test -compiler=gopherjs

You could do:

GOARCH=js go test

Basically, gopherjs can be made the default compiler for js architecture (since gc doesn't support it).

I am currently unaware of other ways to simplify it.

You'll still be able to use gopherjs test of course, that CLI is not going to change.

@bep
Copy link

bep commented May 7, 2016 •
edited
Loading

GOARCH=js go test

Yes, that would work. Or, it would make a fairly good experience. In LIteIDE you can switch environments, so you could have one with GOARCH=js

... and from the shell I could use something like direnv.

dmitshur added a commit that referenced this issue May 8, 2016
@dmitshur
compiler/natives/syscall: Don't import bytes package.
8a047cf 
The real syscall package doesn't import bytes. This keeps the order in
which stdlib packages must be built in for GopherJS consistent with the
real standard library.

Helps #388.
@dmitshur dmitshur mentioned this issue May 8, 2016
Merged
dmitshur added a commit that referenced this issue May 8, 2016
@dmitshur
compiler/natives/syscall: Don't import bytes package.
402d303 
The real syscall package doesn't import bytes. This keeps the order in
which stdlib packages must be built in for GopherJS consistent with the
real standard library.

Helps #388.
dmitshur added a commit that referenced this issue May 8, 2016
@dmitshur
compiler/natives/syscall: Don't import bytes package.
ce68d34 
The real syscall package doesn't import bytes. This keeps the order in
which stdlib packages must be built in for GopherJS consistent with the
real standard library.

Helps #388.
@dmitshur dmitshur mentioned this issue May 31, 2016
Closed
@dmitshur dmitshur self-assigned this Jun 1, 2016
dmitshur added a commit that referenced this issue Jun 5, 2016
@dmitshur
Remove tool command.
2ba3b0e 
It was initially added to support Go language tests. See
da075de#commitcomment-17247656.

Remove it since it's no longer needed, and it's unlikely to be used.

Helps #388.
@dmitshur dmitshur mentioned this issue Jun 5, 2016
Merged
@dmitshur
Copy link
Member Author

dmitshur commented Jun 6, 2016 •
edited
Loading

An update for anyone following this.

This is taking longer because it's a large and complex change. But great news, I'm getting very close to having all the pieces working and ready, and being able to create a PR for this.

A little sneak peek of the upcoming cmd/go command:

image

@joeblew99
Copy link

Really looking forward to kicking the tires on this.

dmitshur added a commit that referenced this issue Jun 13, 2016
@dmitshur
WIP-388: Rewrite gopherjs tool.go to use cmd/go functionality.
0792332 
DO NOT MERGE. This is a PR for review only, it's not in a mergable state because not all components are present. I want to break review of the final PR into smaller pieces, to make it more diffs smaller and manageable.

This change rewrites tool.go to use the functionality of cmd/go with GopherJS compiler support added, which will be bundled here.

Changes:

- watch flag is removed. Supporting it is possible, but difficult at this time and it's hard to test it. It may be brought back later, or maybe it'll be decided that it's out of scope, and another tool can help with watching. With gopherjs serve command available, the need for -watch flag is greatly reduced.

This is a mostly mechanical change, done by hand, so there's a chance for typos or other screw ups, forgetting flags, etc. This is what I want this review to catch.

Helps #388.
@dmitshur dmitshur mentioned this issue Jun 13, 2016
Closed
This was referenced Jun 27, 2016
Closed
Open
This was referenced Jun 27, 2016
Closed
Open
Closed
Closed
@dmitshur dmitshur removed the go1.6 label Jul 25, 2016
@dmitshur dmitshur mentioned this issue Sep 9, 2016
Open
@dmitshur dmitshur mentioned this issue Sep 20, 2016
Merged
@dmitshur dmitshur removed their assignment Dec 12, 2016
@dmitshur
Copy link
Member Author

I'm no longer actively working on this proposal, see #485 (comment) for full details.

@dmitshur
Copy link
Member Author

dmitshur commented May 22, 2017 •
edited
Loading

I'm going to remove the "Proposal Accepted" label from this to reflect current reality.

The implementation effort I started was cancelled (see previous comment). The proposal plan was pretty specific to Go 1.6. By now, Go 1.9 is coming up, and there have been changes to split cmd/go into multiple packages (golang/go#18653). At the same time, we've been making improvements to the internal build tool to fix the issues and bring it up to par to cmd/go functionality.

So, this proposal would need to re-evaluated before being accepted again.

@dmitshur
Copy link
Member Author

dmitshur commented Sep 6, 2017

I'm going to close this now. No one is actively working on this. There've been many minor improvements to the gopherjs tool by now, including:

  • fix for stale packages being incorrectly used
  • support for relative import paths and patterns
  • support for additional test flags such as --count

Which makes the benefit of switching much lower, while the risk and effort stays the same.

As always, should the situation change, this can be re-opened and discussed further.

@dmitshur dmitshur closed this as completed Sep 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
gopherjs-tool Related to the gopherjs tool or its build system (but not the compiler itself). Proposal
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

WIP: Use cmd/go with GopherJS support. gopherjs/gopherjs
4 participants
@dominikh @bep @dmitshur @joeblew99