Port native engine to Mac OSX. r=jonasfj #147
Conversation
walac
force-pushed
the
master
branch
2 times, most recently
from
77de3e8
to
6b558da
Compare
| @@ -143,20 +136,22 @@ func StartProcess(options ProcessOptions) (*Process, error) { | |||
| err = p.cmd.Start() | |||
| } else { | |||
| p.pty, err = pty.Start(p.cmd) | |||
| if options.Stdin != nil { | |||
| if err == nil { | |||
There was a problem hiding this comment.
Why? Don't we want stdout closed?
Note: I see no problem with the other copy attempts failing... errors from these shouldn't cause panic.
There was a problem hiding this comment.
If pty.Start fails, worker will panic with memory access error.
| currentUser, err := user.Current() | ||
| assert.NoError(t, err) | ||
|
|
||
| if currentUser.Username != "root" { |
There was a problem hiding this comment.
This won't work on windows... Hmm, the reason I had the system build constraint was so that we would only run the tests when in a proper context. I guess there could also be contexts where tests are running in root, but we're testing something else and don't want to pollute with garbage users. Considering that the tests are likely to mess up your system, I think hiding then behind a build constraint might be sane.
But I see how auto detecting if the test can run is super nice :)
I guess we don't have to fix this now... We can just leave it like this, maybe add a TODO saying something about how this has to be refactored with testing on windows. Or add the build constraint back...
|
|
||
| return false | ||
| } | ||
|
|
||
| // Note: Constants testGroup, testCat, testTrue, testFalse, testPrintDir, and |
There was a problem hiding this comment.
I love the addition of extra tests for user groups :)
Let's document the testGroups variable here. Just a small thing.
| g, err = user.LookupGroup(groupName) | ||
| require.NoError(t, err) | ||
|
|
||
| gid, err = strconv.Atoi(g.Gid) |
There was a problem hiding this comment.
I think this makes platform specific assumptions again.. We probably shouldn't use user.LookupGroup in these tests.
The idea with the system package is to provide abstractions around the users, groups and processes.
So in system_test.go we test these abstractions, if the abstractions are truly generic, then we shouldn't need to make platform specific assumptions here. Apart from maybe executing different subprograms etc. which is fair game.
But the concepts should be the same.
Does that make sense?
|
|
||
| u, err = CreateUser(homeDir, groups) | ||
| require.NoError(t, err) | ||
| su, err := user.Lookup(u.name) |
There was a problem hiding this comment.
Again this is platform specific.
|
|
||
| uid, err := strconv.ParseUint(su.Uid, 10, 32) | ||
| require.NoError(t, err) | ||
| require.EqualValues(t, u.uid, uint32(uid)) |
There was a problem hiding this comment.
platform specific and accessing private variables...
If we want to do whitebox tests, let's make a system_darwin_test.go file and add darwin specific test cases in there...
Ideally, we can have both generic blackbox tests and platform specific whitebox tests. IMO, the whitebox testing is probably overkill, but it doesn't hurt to have it :)
| @@ -0,0 +1,10 @@ | |||
| package system | |||
|
|
|||
| // test variables | |||
There was a problem hiding this comment.
Ah, this is nice... so much easier to track variables :)
|
@jonasfj I think I fixed all issues. |
If the home folder contains a symbolic link, HasPrefix will fail because the complete path evaluated the symbolic link but prefix didn't. Fix this by expanding prefix symbolic links.
|
Added an extra commit to fix Mac OSX issues. |
Simplify error return statement as advised by gometalinter.
walac
changed the title
There was a problem hiding this comment.
I didn't look much at the unpack and utils stuff...
But I'm okay with taking a look at that later.
Ideally, we could define an interface to be implemented by decompressors... I know we want to add lz4 and zst too. And we'll want to reuse them in streaming mode for things like docker images, qemu images or caches else where...
But like I said, let's refactor that later :)
| if b.payload.Context != "" { | ||
| if err = fetchContext(b.payload.Context, homeFolder.Path()); err != nil { | ||
| b.context.LogError(err) | ||
| return nil, err |
There was a problem hiding this comment.
If there is a problem with the payload you should return a MalformedPayloadError not arbitrary errors... We can't gracefully handling arbitrary errors.
| } | ||
|
|
||
| // TODO: verify if this will harm Windows | ||
| if err = os.Chmod(filename, 0700); err != nil { |
There was a problem hiding this comment.
Add a TODO to abstract this away with a method in the system package... We'll have to add such a method, I'm okay with doing that later...
| @@ -35,6 +38,13 @@ func newSandbox(b *sandboxBuilder) (*sandbox, error) { | |||
| return nil, fmt.Errorf("Failed to temporary folder, error: %s", err) | |||
| } | |||
|
|
|||
| if b.payload.Context != "" { | |||
| if err = fetchContext(b.payload.Context, homeFolder.Path()); err != nil { | |||
There was a problem hiding this comment.
Add a TODO to start downloading at creation of the sandbox builder, and wait for download to finish here.
Add another TODO to make that abortable.
There was a problem hiding this comment.
We actually have no such option. Since we download the context to the user home directory, we cannot start the download before the user is actually created. And we have to wait to the download finishing before start the process. Maybe moving the home directory creation to an earlier stage as well.
There was a problem hiding this comment.
In a world where we cache a reuse downloaded resources, we wouldn't download to the user folder.
Instead we would download to some temporary folder and extract from there every time we need it.
Like said, I think this is just TODOs as the subsystems for this isn't in place yet.
| } | ||
|
|
||
| if err != nil { | ||
| return fmt.Errorf("Error unpacking '%s': %v", filename, err) |
There was a problem hiding this comment.
Don't expose absolute filepath to users... They have no need to know that. The URL is more important.
| @@ -76,6 +86,39 @@ func newSandbox(b *sandboxBuilder) (*sandbox, error) { | |||
| return s, nil | |||
| } | |||
|
|
|||
| func fetchContext(context, destdir string) error { | |||
| filename, err := util.Download(context, destdir) | |||
There was a problem hiding this comment.
Add TODO to use the fetcher subsystem that I have PR for... This is exactly what it is intended for.
Add another TODO to use the cache subsystem when such system has been architected.
|
Regarding the unpack thing, indeed we can make it fancier by applying the factory design pattern. |
A context in a payload represents a file that should be downloaded from a remote URL. If the file is in zip or tar.gz format, it is uncompressed inside user's home directory.
No description provided.