tinygo: -compiler command line argument (e.g. tinygo) #121
Conversation
src/pkg/golang/build.go
Outdated
| s := strings.Fields(string(v)) | ||
| if len(s) < 3 { | ||
| return "", fmt.Errorf("unknown go version, tool returned weird output for 'go version': %v", string(v)) | ||
| if len(s) < 1 { | ||
| return fmt.Errorf(efmt, string(v)) | ||
| } | ||
| return s[2], nil | ||
|
|
||
| compiler := c.Compiler | ||
| compiler.Identifier = s[0] | ||
| compiler.VersionOutput = string(v) | ||
| compiler.IsInit = true | ||
|
|
||
| switch compiler.Identifier { | ||
|
|
||
| case "go": |
There was a problem hiding this comment.
What do you think of refactoring this into smaller chunks and make it a bit more generic, such that it becomes a bit more readable?
type CompilerType int
const (
CompilerGo CompilerType = iota
CompilerTinygo
CompilerUnkown
)
func CompilerTypeFromString(name string) CompilerType {
val, ok := map[string]CompilerType{
"go": CompilerGo,
"tinygo": CompilerTinygo,
}[name]
if ok {
return val
}
return CompilerUnkown
}Further one could implement a parsing function over the Compiler Type so that the parsing logic is put there.
func (c *Compiler) Parse(cType CompilerType) (err error){
switch cType {
case CompilerGo:
// parse
case CompilerTinygo:
// parse
case CompilerUnkown:
// parse
default:
return fmt.Errorf("unkown")
}
}compilerID := CompilerTypeFromString(compiler.Identifier)
if err := compiler.Parse(compilerID); err != nil {
// handle error
}There was a problem hiding this comment.
I would be happy to! Thanks for the feedback!
[I'm just beginning to learn go]
src/pkg/golang/build.go
Outdated
| switch c.Compiler.Identifier { | ||
| case "go": |
There was a problem hiding this comment.
Same thing here, this could be separated from the main logic of the build function to make it more readable.
|
@archie2x if you need said changes
in u-root, do you mind opening a pr? at some point it needs to be merged anyway, right? |
I was going to to, and actually, maybe a larger patch for enabling tinygo build of all cmds in u-root but I saw you already have a PR to make init in u-root/u-root#3062 or am I misunderstanding? I think you're on a better track: putting the link fixes for tinygo in a shared package. For the sake of managing all this, I just created a patch to tinygo to help us identify versions in the sources: tinygo-org/tinygo#4389 |
|
@rminnich @hugelgupf what do you think of this? |
Signed-off-by: Roger Standridge <[email protected]>
Signed-off-by: Roger Standridge <[email protected]>
I marked it as draft because I want to work through the GOROOT. I think, unless overridden by env, it has to be the same as the tinygo 'cached-GOROOT' |
src/pkg/golang/build.go
Outdated
| unique := make(map[string]bool) | ||
| addUnique := func(tag string) { | ||
| if unique[tag] { | ||
| return |
There was a problem hiding this comment.
tags := make(map[string]struct{})
for _, tag := range c.BuildTags {
tags[tag] = struct{}{}
}
for _, tag := range info["build_tags"].([]string) {
tags[tag] = struct{}{}
}
c.BuildTags = maps.Keys(tags) // import "maps"
src/pkg/golang/build.go
Outdated
| } | ||
|
|
||
| efmt := "go-compiler 'version' output unrecognized: %v" |
There was a problem hiding this comment.
here's what I'd do for errors.
var ErrNoGoVersion = fmt.Errorf("go-compiler version output unrecognized")
...
return fmt.Errorf("%w: %s", ErrNoGoVersion, string(v))
This allows using errors.Is with ErrNoGoVersion and keeping the error message the same for the two efmt cases
Signed-off-by: Roger Standridge <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #121 +/- ##
==========================================
- Coverage 62.44% 61.61% -0.84%
==========================================
Files 16 17 +1
Lines 1185 1266 +81
==========================================
+ Hits 740 780 +40
- Misses 445 486 +41
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
src/cmd/makebb/makebb.go
Outdated
| l.Printf("Build environment: %s", env) | ||
| l.Printf("Compiler: %s", env.Compiler.VersionOutput) |
There was a problem hiding this comment.
This looks weird, probably one of my suggestions fits better?
| l.Printf("Build environment: %s", env) | |
| l.Printf("Compiler: %s", env.Compiler.VersionOutput) | |
| l.Printf("Build environment: %s\n", env) | |
| l.Printf("Compiler: %s\n", env.Compiler.VersionOutput) |
Or
| l.Printf("Build environment: %s", env) | |
| l.Printf("Compiler: %s", env.Compiler.VersionOutput) | |
| l.Printf("Build environment: %s Compiler: %s\n", env, env.Compiler.VersionOutput) |
src/pkg/golang/compiler.go
Outdated
| return cmd | ||
| } | ||
|
|
||
| // if go-compiler specified, resolve absolute-path from PATH, |
There was a problem hiding this comment.
| // if go-compiler specified, resolve absolute-path from PATH, | |
| // If go-compiler is specified, resolve the absolute-path from PATH, |
src/pkg/golang/compiler.go
Outdated
|
|
||
| // runs compilerCmd("version") and parse/caches output, to environ.Compiler | ||
| func (c *Environ) CompilerInit() error { | ||
|
|
| compiler.IsInit = true | ||
|
|
||
| switch compiler.Type { | ||
|
|
src/pkg/golang/compiler.go
Outdated
| c.compilerAbs() | ||
|
|
||
| cmd := c.compilerCmd("version") | ||
| v, err := cmd.CombinedOutput() |
There was a problem hiding this comment.
There is no occurance in your code that uses v as raw []bytes returned by cmd.CombinedOutput. Consider making v a string and get rid of all the string(v) if you never need v itself anyways.
src/pkg/golang/compiler.go
Outdated
| } | ||
|
|
||
| func (c Environ) build(dirPath string, binaryPath string, pattern []string, opts *BuildOpts) error { | ||
|
|
|
|
||
| switch c.Compiler.Type { | ||
| case CompilerGo: | ||
|
|
| } | ||
|
|
||
| case CompilerTinygo: | ||
|
|
| // TODO: handle force-rebuild of packages (-a to standard go) | ||
| // TODO: handle EnableInlining |
There was a problem hiding this comment.
I assume this is postponed? I am not sure about the options tinygo may offer here either. I will do some research about this, but independent of this PR, it should not be blocked by this. I think we can squeeze out some size reduction here by setting the right build options.
There was a problem hiding this comment.
I think the issues is more that tinygo already does most of these by default so the existing command line arguments don't map well.
Signed-off-by: Roger Standridge <[email protected]>
help:
I haven't figured out how to propagate this to mkuimage.This allows specifying the compiler explicitly in makebb and handles peculiarities of tinygo. It works by resolving and caching the path to the go-compiler then queries tinygo for its hidden go-build-tags and propagates them to package lookup / install. This allows the pruning algorithm to correctly add / remove files when syncing dependent packages.
Usage:
Resulting busybox is significantly smaller. I compiled a slightly more complicated project than e.g. u-root/cmds/core/{true,false} to test package resolution.
Standard go:
tinygo:
Needs: