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.

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

Allows compilation of code using debug.BuildInfo and show correct tinygo version #4343

Merged
merged 4 commits into from
Jul 20, 2024
Merged

Allows compilation of code using debug.BuildInfo and show correct tinygo version #4343

merged 4 commits into from
Jul 20, 2024

Conversation

ldemailly
Copy link
Contributor

@ldemailly ldemailly commented Jul 16, 2024 •
edited
Loading

That let's me build a 245k tinygo target grol-io/grol#37
(also needed some change in fortio.org/log)

Many thanks to @ydnar and @dankegel for their help and guidance.

dkegel-fastly
dkegel-fastly approved these changes Jul 17, 2024
View reviewed changes
Copy link
Contributor

@dkegel-fastly dkegel-fastly left a comment

Choose a reason for hiding this comment

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

LGTM

ydnar
ydnar reviewed Jul 17, 2024
View reviewed changes
src/runtime/debug/debug.go Outdated Show resolved Hide resolved
src/runtime/debug/debug.go Show resolved Hide resolved
src/runtime/debug/debug.go Outdated
//
// Not implemented.
func (bi *BuildInfo) String() string {
return "<build info placeholder>\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you implement this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

given everything else is empty in the struct, what would that do?

could we just import the real code from big go?

Copy link
Contributor

Choose a reason for hiding this comment

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

"else"? It's not printing anything at all. It could at least return "go. " + GoVersion, couldn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ic the first line sure, I didn't notice because when I use buildinfo I print the GoVersion separately let me add something

btw should it be

go	tinygo0.33.1

or

tinygo tinygo0.33.1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doing the former, copying the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dkegel-fastly so I copied the real code but... it's ugly... what is the general philosophy/must not be the first time tinygo is just a small variant of an existing go file, can you import+patch or some other way to avoid copy pasta (this is more for my education than anything specific for this PR - hopefully it's ok now with 41dd04a )

@ldemailly ldemailly mentioned this pull request Jul 17, 2024
Merged
@ldemailly
Copy link
Contributor Author

ldemailly commented Jul 17, 2024 •
edited
Loading

output comparison (through fortio.org/version) - edited for latest commit:

$ ./grol.tiny buildinfo
dev  tinygo0.33.0-dev arm64 darwin
go	tinygo0.33.0-dev
$ ./grol buildinfo
dev  go1.22.5 arm64 darwin
go	go1.22.5
path	grol.io/grol
mod	grol.io/grol	(devel)	
dep	fortio.org/cli	v1.7.0	h1:w+uXZLGi4t3Vn/BvbeMuSw84Z1pvNPG9HqeGfpP68cc=
dep	fortio.org/log	v1.15.0-pre4	h1:jAr6cn7AHhKBBdD6M2WlCqNWnePyCMp9A6++o0Zl0jY=
dep	fortio.org/struct2env	v0.4.1	h1:rJludAMO5eBvpWplWEQNqoVDFZr4RWMQX7RUapgZyc0=
dep	fortio.org/version	v1.0.4	h1:FWUMpJ+hVTNc4RhvvOJzb0xesrlRmG/a+D6bjbQ4+5U=
dep	github.com/kortschak/goroutine	v1.1.2	h1:lhllcCuERxMIK5cYr8yohZZScL1na+JM5JYPRclWjck=
build	-buildmode=exe
build	-compiler=gc
build	-tags=no_net,no_json
build	-trimpath=true
build	CGO_ENABLED=0
build	GOARCH=arm64
build	GOOS=darwin
build	vcs=git
build	vcs.revision=9396c9b051e054a7783f75195aac0084f20911d2
build	vcs.time=2024-07-16T07:02:48Z
build	vcs.modified=true

ydnar
ydnar reviewed Jul 17, 2024
View reviewed changes
src/runtime/debug/debug.go Outdated
@@ -1,6 +1,13 @@
// Package debug is a dummy package that is not yet implemented.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this comment be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's still kinda dummy - I was wondering about the comment

// Not implemented.
func ReadBuildInfo()

and figured to leave it as it's... only giving an empty one (no modules, no settings etc) except for GoVersion

@ldemailly
Copy link
Contributor Author

I see the approvals, thanks, would one of you merge, I can't (wouldn't anyway)

dkegel-fastly
dkegel-fastly reviewed Jul 18, 2024
View reviewed changes
src/runtime/debug/debug.go
@@ -58,3 +66,60 @@ type Module struct {
func SetGCPercent(n int) int {
return n
}

// Start of stolen from big go. TODO: import/reuse without copy pasta.
Copy link
Contributor

Choose a reason for hiding this comment

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

I do believe a copyright notice should be copied here...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isn’t this covered by https://github.com/tinygo-org/tinygo/blob/release/LICENSE#L3 ?

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 the copyright folks like to have a greppable marker in each file.

I like the way runtime/timer.go does it: copy the three top llines from the file you're copying, and change "Copyright" to "Portions copyright".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I was looking for an example, thanks for mentioning src/runtime/timer.go

Done

@ldemailly
Copy link
Contributor Author

ldemailly commented Jul 18, 2024 •
edited
Loading

hmm I messed up my fork trying to start a new PR earlier (about TLS) let me fix this (hopefully) fixed PTAL

@ldemailly
Copy link
Contributor Author

The previous build https://github.com/tinygo-org/tinygo/actions/runs/9977696304 of size difference, before llvm18 got broken, passed, so can we merge this ? (only difference is the comment at the top of the file, since that build)

@ldemailly
Copy link
Contributor Author

ping

@deadprogram
Copy link
Member

@ldemailly please rebase against the latest dev and this PR should pass CI now.

@deadprogram
Copy link
Member

Actually, I can just squash/merge this now, I think.

Thank you very much @ldemailly for working on this and to @ydnar and @dkegel-fastly for guidance and reviews.

@deadprogram deadprogram merged commit 89340f8 into tinygo-org:dev Jul 20, 2024
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dkegel-fastly dkegel-fastly dkegel-fastly approved these changes

@ydnar ydnar ydnar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ldemailly @deadprogram @ydnar @dkegel-fastly