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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce System::User and System::Group #7725

Merged
merged 47 commits into from Jul 27, 2019

Conversation

@woodruffw
Copy link
Contributor

commented Apr 29, 2019

Introduces the System::User and System::Group classes, which in turn delegate to the Crystal::System::User and Crystal::System::Group classes.

This needs documentation and unit tests, but I'll hold off on the latter until a few maintainers 馃憤 this general structure/approach.

Example usage:

require "system/group"
require "system/user"

group = System::Group.from_id(1000_u32)
puts group.name, group.members

user = System::User.from_username(group.members.first)
puts user.directory, user.shell

See #5627, #5615

Show resolved Hide resolved src/system/user.cr Outdated
Show resolved Hide resolved src/crystal/system/unix/user.cr Outdated
@straight-shoota

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Just to be sure: This essentially extracts a part of #5627 and only adds the System::User and System::Group types?

Looks good. Specs and doc would be awesome 馃憤

@woodruffw

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

Just to be sure: This essentially extracts a part of #5627 and only adds the System::User and System::Group types?

Correct! My thought here was to keep the initial changeset small, and work to expand the API/interaction with other classes (File::Info and Process come to mind) in subsequent PRs.

Specs and doc would be awesome 馃憤

Will do! I'll put some initial specs in this evening (EST).

@woodruffw

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

Alright, added initial specs.

Some notes:

  • Unless we augment the testing environment to add special users and groups, there's no good testing story for the Group#password, Group#members, User#password, User#group_id, User#directory, or User#shell. The User attributes could be tested by adding something like System::User.each_user that wraps getpwent_r, but that's a nonstandard call. Not a huge deal, just something to consider.
@woodruffw

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

Failure on linux32 looks unrelated:

 1) TCPServer .new using IPv4 reuse_port raises when not binding with reuse_port

     Failure/Error: expect_raises_errno(Errno::EADDRINUSE, {% if flag?(:gnu) %}"listen: "{% else %}"bind: "{% end %}) do



       Expected Errno with "listen: ", got #<Errno: bind: Address already in use> with backtrace:
@straight-shoota

This comment has been minimized.

Copy link
Member

commented May 1, 2019

Splitting #5627 into separate pieces is a good idea 馃憤

Did you take a look at how other implementations test this? I don't think setting up a specific environment for these specs is feasible. So we need to accept that some features can't be tested effectively. But they're all in all pretty simple wrappers to OS features, so this shouldn't be a big deal. Make sure to add typeof calls though to make sure the code is at least valid even if it's not run.

But you could take some inspiration from Golang's test suite for example (e.g. https://github.com/golang/go/blob/master/src/os/user/user_test.go) on how others implement tests for this.

@woodruffw

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

But you could take some inspiration from Golang's test suite for example (e.g. https://github.com/golang/go/blob/master/src/os/user/user_test.go) on how others implement tests for this.

Thanks! Looks like Golang uses the running process's uid and gid (presumably via getuid/getgid) to create objects with testable state. We could duplicate this approach by adding Process.user and Process.group, and then yanking the relevant names/IDs out of those objects. But perhaps that's better for a future PR, to avoid ballooning this one any more.

Either way, I'll add type checks now 馃憤

@straight-shoota

This comment has been minimized.

Copy link
Member

commented May 1, 2019

But perhaps that's better for a future PR, to avoid ballooning this one any more.

Sounds like a good idea. But in the specs you could simply use `id -un` and `id -gn` for now.

@woodruffw

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

But in the specs you could simply use `id -un` and `id -gn` for now.

Ah, totally forgot I can shell out in tests. Thanks! I'll use those.

@j8r

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

You can use struct instead of class.
Why having private getters for sys_group and sys_user?

@woodruffw

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

You can use struct instead of class

馃憤

Why having private getters for sys_group and sys_user?

I might be mistaken about this, but my understanding is that the Crystal::System hierarchy should never be exposed directly to users -- it's purely for filling out the actual user-facing classes. That's why I made the two private, but I'm 馃憤 on exposing them publicly if that's preferred.

Edit: I'm currently juggling a few other things, so it'll probably be a few days before I address the struct recommendation/any additional feedback.

@j8r

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

@woodruffw the questiom wasn't really why they are private, this was more about why private getter instead of a regular ivar @sys_user. Only private def initialize(@sys_user : Crystal::System::User should be enough (same for sys_group)

@woodruffw

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

@j8r Ah, gotcha. Yes, a regular ivar would be just fine. Will change.

@woodruffw

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

Hmm, looks like id -un chokes on (some) of the linux CIs:

id: cannot find name for user ID 1001

Error in spec/std_spec.cr:2: while requiring "./std/**"

Will investigate further tomorrow.

@woodruffw

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

Guessing it has something to do with the -u 1001 in this docker invocation:

docker run --rm -t -u 1001 -v /home/circleci/project:/mnt -w /mnt -e CRYSTAL_CACHE_DIR=/tmp/crystal crystallang/crystal:0.28.0-build linux64 /bin/sh -c 'make std_spec clean threads=1'
@straight-shoota

This comment has been minimized.

Copy link
Member

commented May 5, 2019

@woodruffw

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

Gotcha, thanks for that! I'll apply the same fix.

@woodruffw woodruffw changed the title [WIP] Introduce System::User and System::Group Introduce System::User and System::Group May 5, 2019

@straight-shoota
Copy link
Member

left a comment

Overall this looks good and is a much better chewable chunk than #5627. This helps a lot to move this feature further.

However, there are some derivations from the original implementation that I don't think are an improvement (like password property), and some neat features missing (such as #to_s and def_equals_and_hash on the new types).

A more general problem is the exact materialization of the public API, especially which properties should be exposed at all. We've touched this in previous discussions but not in depth. And I think this is a sub topic which deserves a dedicated discussion and not clutter this PR (as it's independent of the concnrete implementation). For this design process I've opened #7738.

For now, I have only added a few high-level review notes here. We should settle on the API first and then look deeper into the details of implementation.

Show resolved Hide resolved spec/std/system/group_spec.cr Outdated
Show resolved Hide resolved src/system/group.cr Outdated
Show resolved Hide resolved src/crystal/system/unix/group.cr Outdated
@woodruffw

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

However, there are some derivations from the original implementation that I don't think are an improvement (like password property), and some neat features missing (such as #to_s and def_equals_and_hash on the new types).

Went ahead and removed the password property from both User and Group. I'll add to_s in a bit.

Re: def_equals_and_hash: I think just the user/group ID should be sufficient, at least for POSIX. Thoughts?

I'll also follow up on the general design process in #7738.

@woodruffw

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

Anything else this PR needs? We should definitely continue the discussion in #7738, but I think (unless I'm mistaken) that there's nothing objectionable about the current implementation/API.

@straight-shoota

This comment has been minimized.

Copy link
Member

commented May 16, 2019

Looks good so far, only the data types of group and user ids should be String instead of platform-specific types. Also, from #7738 I'd like to have User#username return the username and User#name the real/display name.

@woodruffw

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

Bleh, this got lost in the stack. I'll catch up on this and make the suggested changes latter this week/weekend.

@woodruffw woodruffw force-pushed the woodruffw-forks:users-and-groups branch from fce0f46 to 3e84986 Jun 10, 2019

@woodruffw

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

Looks good so far, only the data types of group and user ids should be String instead of platform-specific types. Also, from #7738 I'd like to have User#username return the username and User#name the real/display name.

Instead of making uid and gid into strings, might it make sense to only expose them as part of the API for :unix targets? That way we get the best of both worlds: Linux/macOS/BSD users can treat UIDs and GIDs as the integers they are, and any Crystal code compiled for Windows that uses them will fail at compile time. We could then add the appropriate (feature-gated) methods for Windows as well, overlapping names where appropriate.

I got this idea from looking at the File::Info source for Win32: instead of just failing at compile time when someone tries to use File::Info#owner or #group on a system where those don't make sense, we currently return 0_u32. As a result, a user attempting to re-build their Crystal project on Windows will (1) miss the semantic bug of using a method on a platform that it doesn't make sense for, and (2) get a very confusing result (0 is usually root's UID).

In any case, I'll change #name to #username and extract the full name from the GECOS field.

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

The standard library is supposed to be portable. When a program runs on Linux, it should just run on MacOS, BSD, Windows by default. A developer should not have to worry about platform-specifics.

Thus the standard library needs to provide an appropriate abstraction layer. Platform-specific APIs can be provided by shards.

So the gid/uid should be encoded as strings. That's the typical platform-independent implementation. And I don't think there is any downside, except that POSIX developers are used to use integers. When writing platform-specific code interfacing with POSIX APIs, you'll need to convert the datatype. But that's hardly a big issue.

Show resolved Hide resolved src/system/user.cr Outdated
@woodruffw

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

The standard library is supposed to be portable. When a program runs on Linux, it should just run on MacOS, BSD, Windows by default. A developer should not have to worry about platform-specifics.

Thus the standard library needs to provide an appropriate abstraction layer. Platform-specific APIs can be provided by shards.

So the gid/uid should be encoded as strings. That's the typical platform-independent implementation. And I don't think there is any downside, except that POSIX developers are used to use integers. When writing platform-specific code interfacing with POSIX APIs, you'll need to convert the datatype. But that's hardly a big issue.

All fair points, and I ultimately want to use these features so I won't argue too much here 馃槈

But as a general observation: I think users are sometimes best served by a standard library that isn't cross-platform, as least not when "cross-platform" is defined as "the same calls on all platforms." There are some things (user and group management here, but also fine-tuned socket and I/O control, extended permissions, etc.) that are really nice to have in a standard library, but don't fit cleanly into abstract, platform-agnostic hierarchies.

I think Crystal is uniquely positioned to ameliorate a lot of the conventional hassle of writing platform-specific code in a platform-agnostic codebase: DCE, the type system, hygenic macros, feature flags, etc. make it possible to do platform-specific things cleanly, and I think users would appreciate a standard library that stops the compilation on a platform where (e.g.) File::Info#owner is meaningless rather than implicitly doing the wrong thing.

In any case, I'll change uid and gid to Strings in a moment.

woodruffw added some commits Jul 26, 2019

@straight-shoota
Copy link
Member

left a comment

I won't approve Group#members. But the rest looks fine 馃憤

@asterite

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

Yes, I'm also not sure about Group#members, mainly because it's not there in Go so I'm sure there must be a good reason (maybe there's no such thing in Windows or it's expensive to compute, and with this way we'd be making this call always expensive there?).

We can always add it later if we find out there's no problem.

Also, we can always remove it later if we find a problem, after all we are 0.x.

@asterite
Copy link
Member

left a comment

Looks good!

I think we should move forward with this so people can start using it and experimenting with it. We can always tweak the API later and it's easier with actual usage.

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Jul 26, 2019

I'm sure there must be a good reason

I'm now pretty convinced that this is for memory implications. Imagine iterating a list of N files which all belong to a group of M users. When retrieving the group for each file, this will allocate N arrays of M System::User instances. And you'll most likely don't ever need any of them.

@woodruffw

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

Okay, nixed #members.

@straight-shoota
Copy link
Member

left a comment

GTG 馃憤

@asterite

This comment has been minimized.

Copy link
Member

commented Jul 27, 2019

Cool! Should this make it into 0.30.0?

@bcardiff bcardiff added this to the 0.30.0 milestone Jul 27, 2019

@bcardiff bcardiff merged commit 7f51817 into crystal-lang:master Jul 27, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bcardiff

This comment has been minimized.

Copy link
Member

commented Jul 27, 2019

Yes, it's a new feature, it's safe to be added.

Thanks @woodruffw and everybody involved

@woodruffw woodruffw deleted the woodruffw-forks:users-and-groups branch Jul 27, 2019

@woodruffw

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

Yay! Many thanks to all who reviewed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.