Skip to content

os/user.Lookup #4217

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
Closed
wants to merge 3 commits into from
Closed

os/user.Lookup #4217

wants to merge 3 commits into from

Conversation

leongross
Copy link
Contributor

Add support for User lookup via os/user.Lookup()

@leongross
Copy link
Contributor Author

@deadprogram any thoughts on this?

@leongross
Copy link
Contributor Author

@deadprogram do you have any recommendations on what to improve or does this look good to you?

@leongross
Copy link
Contributor Author

@aykevl maybe you can have a short look at this?

@10000TB
Copy link

10000TB commented Apr 30, 2024

ping for review

@deadprogram
Copy link
Member

Just waiting for #4027 to land before any more changes to this part of the code.

@leongross leongross force-pushed the os/user/Lookup branch 3 times, most recently from 6c8cf91 to d9ef0e1 Compare May 10, 2024 11:28
@leongross
Copy link
Contributor Author

leongross commented May 10, 2024 •
edited
Loading

for some reason smoke test test-llvm15-go118 fails again

@aykevl
Copy link
Member

aykevl commented May 13, 2024

for some reason smoke test test-llvm15-go118 fails again

That's #4206

aykevl
aykevl reviewed May 13, 2024
View reviewed changes
Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

I guess this was mostly copied from upstream Go, correct?
The code looks reasonable, though I would prefer if the test was actually being run (you can add it to GNUmakefile, see TEST_PACKAGES_FAST and related variables). Also, what about MacOS/Windows/baremetal support? Does it just fail to compile? I'm worried we might be breaking some existing programs that currently work.

@leongross leongross force-pushed the os/user/Lookup branch 2 times, most recently from 5a14b28 to 2f1996f Compare May 21, 2024 12:58
@leongross
Copy link
Contributor Author

leongross commented May 21, 2024 •
edited
Loading

Not quite sure why the windows build fails here: ld.lld: error: undefined symbol: notARealFunction

@leongross
Copy link
Contributor Author

leongross commented Jun 4, 2024 •
edited
Loading

commit 7b7601d added stubs, but we should still get this functionality in @deadprogram

@leongross
Copy link
Contributor Author

Since I did not touch the rp2040 specific code, it looks like yet another CI error

/go/bin/tinygo build -size short -o test.hex -target=nano-rp2040 examples/rtcinterrupt
unexpected fault address 0x27cc
fatal error: fault
[signal SIGSEGV: segmentation violation code=0xfffffffffffffffa addr=0x27cc pc=0x6e9d62]

@leongross
Copy link
Contributor Author

@deadprogram @aykevl the CI race again, ready to merge?

@leongross leongross requested a review from aykevl June 6, 2024 15:47
@leongross
Copy link
Contributor Author

@aykevl @deadprogram @dgryski @10000TB

@10000TB
Copy link

10000TB commented Jun 11, 2024

anyone with approval power care to approve ?

@deadprogram
Copy link
Member

@leongross can you please rebase this PR against the latest dev? Thanks!

@leongross
Copy link
Contributor Author

@deadprogram rebase done

@leongross
Copy link
Contributor Author

@aykevl

@10000TB
Copy link

10000TB commented Jun 25, 2024

Looking for an approval ! TY

@leongross
Copy link
Contributor Author

@deadprogram @aykevl

deadprogram
deadprogram approved these changes Jul 2, 2024
View reviewed changes
Copy link
Member

@deadprogram deadprogram left a comment

Choose a reason for hiding this comment

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

Thanks for making the requested changes @leongross I will squash/merge very soon.

@aykevl
Copy link
Member

aykevl commented Jul 2, 2024

Oh sorry I totally forgot about this PR!
@leongross what do you think about #4324? It defers to the Go stdlib for the os/user package by setting the osusergo build tag, which means we have less to maintain in TinyGo.

@aykevl aykevl mentioned this pull request Jul 2, 2024
Merged
@leongross
Copy link
Contributor Author

If the usergo tag works as I would suspect this should resolve this PR and any further implementations, looks really promising!

@deadprogram
Copy link
Member

@leongross @aykevl so that means #4324 would replace this PR, correct?

@deadprogram
Copy link
Member

@leongross @aykevl so that means #4324 would replace this PR, correct?

Just wondering... 😸

@leongross
Copy link
Contributor Author

@leongross @aykevl so that means #4324 would replace this PR, correct?

Just wondering... 😸

I hope so :) This would be a good generic solution, I cannot think of any use case atm where the implementation covered in this PR would make a difference.

@leongross
Copy link
Contributor Author

#4324 is successfully merged so this can be closed.

@leongross leongross closed this Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deadprogram deadprogram deadprogram approved these changes

@aykevl aykevl Awaiting requested review from aykevl

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
@leongross @10000TB @deadprogram @aykevl