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.

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

os: net.newUnixFile #4517

Merged
merged 1 commit into from
Oct 18, 2024
Merged

os: net.newUnixFile #4517

merged 1 commit into from
Oct 18, 2024

Conversation

leongross
Copy link
Contributor

@leongross leongross commented Oct 11, 2024

Building the u-root cmdlet sluinit with my experimental net PR #4498 following error occurs:

/usr/local/go/src/net/fd_unix.go:205: linker could not find symbol net.newUnixFile

This PR resolves the issue of the missing function net_newUnixFile and links it into the net package.

Since this package implements functionality for the linux net package, this code can currently not really be tested (see #4498).

@leongross
Copy link
Contributor Author

@aykevl @deadprogram @dgryski

@leongross
Copy link
Contributor Author

Since the wasm CI seems to be broken in other PRs not touching this as well, I presume this PR did no changes to crash it (see #4521)

@leongross
Copy link
Contributor Author

@ydnar

@deadprogram
Copy link
Member

The fix for nix is in... or at least for the CI. Please rebase against latest dev.

Signed-off-by: leongross <leon.gross@9elements.com>
@leongross
Copy link
Contributor Author

@deadprogram @aykevl ci is green now, would appreciate a review

Copy link
Member

@dgryski dgryski left a comment

Choose a reason for hiding this comment

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

Approving to allow this to be merged and not block the net stuff, but I have concerns that we're drifting from Go's requirements for this function.

@deadprogram
Copy link
Member

Thanks for the addition @leongross and to @dgryski and @rminnich for review. Now merging.

@deadprogram deadprogram merged commit 01dac8b into tinygo-org:dev Oct 18, 2024
17 checks passed
@leongross
Copy link
Contributor Author

I am aware of your concerns @dgryski and I think they are valid. This is just a POC workaround, as @aykevl already pointed out somewhere in the near future we have to consider our own implementation of the internal/poll package. But for now the stub is very helpful, thanks also to @rminnich

@deadprogram
Copy link
Member

Not sure why, but the Windows CI is now failing consistently after this PR was merged, even though the PR itself was passing.

See https://github.com/tinygo-org/tinygo/actions/runs/11408347958/job/31767355704#step:17:587

I have restarted it already 3 times...

Any ideas?

@deadprogram
Copy link
Member

It finally got thru the first step. Seems like some race condition and not related to this PR. Sorry about the noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants