You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The net/http directory override hides Go's net/http/cookiejar package.
Merge net/http to expose the standard cookie jar. Keep TinyGo's HTTP files and subpackage overrides. Add overrides for cgi and fcgi so these unsupported packages remain unavailable. Initialize src/net in the Nix job.
Tests:
Walk the real src/net/http tree and check override coverage. A directory override includes its full subtree.
Check that the merge selects TinyGo HTTP files and subpackages, selects Go's cookie jar, and excludes Go's cgi and fcgi.
Run a host test that stores a secure cookie and refuses it over HTTP. Mark this test as host-only.
Local validation: both loader tests passed in a harness with the unchanged production merge functions. Checks with a missing override failed as expected. The cookie test passed with Go. Full local loader tests could not build because LLVM headers are absent.
Thanks @yohimik I have a few important notes edited from an automated review.
This is the biggest one. The PR description is mostly about the Crier and Dispat releases, fork CI and binary sizes. For a six-file loader change this hides the content. Please shorten it to the problem, the change and the specific tests. If you follow AGENTS.md it should help to reduce the amount of text. The large amount of text makes it nearly impossible to review changes. Despite that, here are a few specific comments.
With net/http/ merged, only TinyGo files are linked into the merged GOROOT. A TinyGo subdirectory appears only if it has an entry in pathsToOverride. Because src/net is a submodule with its own release cycle, a new subpackage added there would disappear from the merged GOROOT with no error. The new test uses a synthetic tree, so it cannot find this. Please add a test that walks the real src/net/http and checks that each subdirectory has an override entry.
Go's net/http/cgi and net/http/fcgi now also come through the merge, but they do not build against the TinyGo crypto/tls. An import of net/http/cgi fails with net/http/cgi/child.go:109:32: unknown field HandshakeComplete in struct literal of type tls.ConnectionState, and net/http/fcgi fails in the same way because it imports cgi. Before this change the message was that the package is not in std. Please either add override entries for these two directories, or record this change of behavior in the PR description.
In main_test.go, the reason for the second runPlatTests call is not obvious. Please add a short comment that cookiejar.go is a host-only test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
net/httpdirectory override hides Go'snet/http/cookiejarpackage.Merge
net/httpto expose the standard cookie jar. Keep TinyGo's HTTP files and subpackage overrides. Add overrides forcgiandfcgiso these unsupported packages remain unavailable. Initializesrc/netin the Nix job.Tests:
src/net/httptree and check override coverage. A directory override includes its full subtree.cgiandfcgi.Local validation: both loader tests passed in a harness with the unchanged production merge functions. Checks with a missing override failed as expected. The cookie test passed with Go. Full local loader tests could not build because LLVM headers are absent.