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/user: add solaris getgroups support #14709

Open
bradfitz opened this issue Mar 8, 2016 · 20 comments
Open

os/user: add solaris getgroups support #14709

bradfitz opened this issue Mar 8, 2016 · 20 comments

Comments

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Mar 8, 2016

Solaris is broken after the os/user groups change:

http://build.golang.org/log/26a14fbc0126ab1ed65eaeffe18362689b13a54f

...
net
os/user
# os/user
Undefined           first referenced
 symbol                 in file
getgrouplist                        $WORK/os/user/_obj/lookup_unix.cgo2.o
ld: fatal: symbol referencing errors. No output written to $WORK/os/user/_obj/_cgo_.o
collect2: error: ld returned 1 exit status
crypto/x509
net/textproto
log/syslog
...

Let me know if you need help testing on Solaris. It's not yet a trybot, due to lack of time.

@bradfitz bradfitz added this to the Go1.7 milestone Mar 8, 2016
@bradfitz bradfitz added the OS-Solaris label Mar 8, 2016
@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Mar 8, 2016

Apparently Solaris didn't have getgrouplist until recently. I found elsewhere that "getgrouplist() is provided by the latest Solaris 11.2 SRU 11.2.10.5.0, released 15 May 2015. It will be available in all future updates/releases." But apparently our builders don't have that.

@4ad, advice?

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Mar 8, 2016

Actually, we should just not try to build it & test it. I'll send out a CL. We can keep this bug open to add support later.

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Mar 8, 2016

Or rather, I'll wait for @zombiezen to finish his pending CLs, since my change would necessarily split lookup_unix.go into two halves probably (user lookup vs group lookup, with different build tags), or maybe add a group_solaris.go file with an import "C" and a fake getgrouplist impl in it just for Solaris.

Ross, let me know if you prefer to tackle this.

@4ad
Copy link
Member

@4ad 4ad commented Mar 8, 2016

We will have to implement getgrouplist for Solaris ourselves. In the meantime we can stub it.

@zombiezen
Copy link
Contributor

@zombiezen zombiezen commented Mar 8, 2016

I can roll in the stub as part of https://golang.org/cl/20348, since I'm already splitting out the OS-specific functionality.

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Mar 8, 2016

Great, thanks.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 8, 2016

CL https://golang.org/cl/20348 mentions this issue.

@zombiezen
Copy link
Contributor

@zombiezen zombiezen commented Mar 8, 2016

Just to point out, the other group-related functionality should still work on Solaris (let me know if it doesn't). getgrouplist is annoyingly non-standard. I'm contemplating whether the internal listGroups function should just parse /etc/groups itself in the future because it will ironically be more portable.

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Mar 8, 2016

Parsing /etc/groups as a fallback seems fine too.

@gopherbot gopherbot closed this in f128b54 Mar 8, 2016
@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Mar 8, 2016

Repurposing this bug to add Solaris support, since this is the bug number references in the t.Skip text now.

@bradfitz bradfitz reopened this Mar 8, 2016
@bradfitz bradfitz changed the title os/user: solaris broken by groups change os/user: add solaris getgroups support Mar 8, 2016
@binarycrusader
Copy link
Contributor

@binarycrusader binarycrusader commented Mar 8, 2016

Some versions of Solaris support this function, so ideally, if there is a sanctioned way to see if we can import the symbol from libc, and if it's nil, then fallback to the other functionality, that seems like the best option.

@minux
Copy link
Member

@minux minux commented Mar 9, 2016

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 10, 2016

We are talking about cgo here, and my understanding is that Solaris doesn't support internal linking anyhow. So I think we could use a weak reference in the C code.

@binarycrusader
Copy link
Contributor

@binarycrusader binarycrusader commented Mar 10, 2016

The internal linker works just fine on Solaris. Not sure I understand?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 10, 2016

Sorry, I expect it is me that does not understand.

@binarycrusader
Copy link
Contributor

@binarycrusader binarycrusader commented Mar 10, 2016

I guess I was assuming that cgo import dynamic would simply assign nil for symbols it can't find:

//go:cgo_import_dynamic libc_Getgrouplist getgrouplist "libc.so"

My assumption was we could just check for nil in the wrapper function, if not nil, call the function, and if nil, fallback to other logic.

But I don't know if that's possible.

@minux
Copy link
Member

@minux minux commented Mar 10, 2016

@rsc rsc modified the milestones: Go1.8, Go1.7 May 18, 2016
@zombiezen zombiezen removed their assignment Sep 15, 2016
@quentinmit quentinmit added the NeedsFix label Oct 10, 2016
@rsc rsc modified the milestones: Unplanned, Go1.8 Oct 19, 2016
@matejb
Copy link
Contributor

@matejb matejb commented Sep 27, 2017

Stumbled on this issue yesterday while working on the application for Illumnos based OS.
And made the in-app version of the function that parses /etc/group.

And I see there are existing private functions for /etc/group parsing inside user package.
I can use these to make User.GroupIds work for Solaris.
If that would be acceptable?

@4ad
Copy link
Member

@4ad 4ad commented Sep 27, 2017

No, we need to do what @minux said in his last reply.

@andy-js
Copy link
Contributor

@andy-js andy-js commented Aug 6, 2020

I'm not sure why nobody suggested implementing this using getgrent. There's no actual need to have getgrouplist.

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

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.