-
Notifications
You must be signed in to change notification settings - Fork 890
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
Go module support #941
Go module support #941
Conversation
b98a76c
to
7301dfc
Compare
This PR is now ready for review. The first two commits get things ready, they are not directly related to Go module support. The third switches to a custom GOROOT which is cached between invocations. The fourth actually replaces the custom loader with (indirectly) |
4ae3f73
to
f7b2dc0
Compare
Hi @aykevl this PR has a merge conflict to resolve, please. |
Fixed the merge conflict. Additionally, by adding the TinyGo version to the cache key it should be very unlikely that the wrong cache key is used in releases. During development, it may still be necessary to run |
Yet another merge conflict @aykevl can you please resolve? Thanks. |
This is necessary to avoid a circular dependency in the loader (which soon will need to read the Go version) and because it seems like a better place anyway.
Done. This conflict was probably introduced with #1125. |
Seems like something went wrong with the linux build? https://app.circleci.com/pipelines/github/tinygo-org/tinygo/2972/workflows/e8dc6e29-f0e7-4ad7-90a9-0ab20ec9b77c/jobs/13574/steps |
Here is the specific problem:
|
This is needed to make it available to more packages, for caching purposes. For caching, the version itself may not be enough during development. But for regular releases, the version provides some protection against accidentally using a cache entry that is invalid in a newer version.
It was broken for quite some time without anybody noticing...
This commit changes the way that packages are looked up. Instead of working around the loader package by modifying the GOROOT variable for specific packages, create a new GOROOT using symlinks. This GOROOT is cached for the specified configuration (Go version, underlying GOROOT path, TinyGo path, whether to override the syscall package). This will also enable go module support in the future. Windows is a bit harder to support, because it only allows the creation of symlinks when developer mode is enabled. This is worked around by using symlinks and if that fails, using directory junctions or hardlinks instead. This should work in the vast majority of cases. The only case it doesn't work, is if developer mode is disabled and TinyGo, the Go toolchain, and the cache directory are not all on the same filesystem. If this is a problem, it is still possible to improve the code by using file copies instead. As a side effect, this also makes diagnostics use a relative file path only when the file is not in GOROOT or in TINYGOROOT.
This commit replaces the existing ad-hoc package loader with a package loader that uses the x/tools/go/packages package to find all to-be-loaded packages.
I hope I really fixed it this time. |
Finally the tests pass! (Although TinyHCI appears to be stuck?) |
All the tests are passing now. |
I am just wondering how to test this exactly. @aykevl can you please explain? |
Module support itself is not directly tested. But because all compilations (both the tests in main_test.go and the smoke tests) use the new system that also supports Go modules, that might be enough. For example: the list of Go files to compile is provided by the Go command ( |
Well, it does appear to be working as expected in all cases. I am going to merge, and if any new issues appear as a result, we can address them. Great work on this @aykevl |
This is more of a mockup than anything serious. I want to see whether creating a new GOROOT per build is feasible. Right now creating such a GOROOT costs about 4ms on my system (after a warmed-up cache) so is not a significant overhead per build.
I'm not sure whether the current approach (with symlinks) is going to be viable on Windows.
The first commit adds the
go list
command, which might be enough to support TinyGo in IDEs such as VS Code.