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

Enable build on solaris #4115

Merged
merged 3 commits into from Apr 23, 2017
Merged

Enable build on solaris #4115

merged 3 commits into from Apr 23, 2017

Conversation

@ptribble
Copy link
Contributor

ptribble commented Apr 13, 2017

Description

The minio server fails to build on solaris.

To enable solaris, I have enabled solaris for rlimit-file and rlimit-memory, and added and implementation of stats_solaris.go

Motivation and Context

With these changes, minio builds on solaris.

How Has This Been Tested?

A build using 'go get -u' produces a minio executable on solaris

I have run and tested minio server - I can list buckets, list files, upload and download files, create buckets.

(The alternative build using checkout and make doesn't work properly on solaris. That's a slightly different issue unrelated to code or functionality that I would also like to fix.)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

I'm not quite sure how to run the tests. I have ensured that 'make verifiers' is clean, though. And I'm fairly sure that I haven't done anything that will affect any other platforms.

#include <unistd.h>
*/
import (
"C"

This comment has been minimized.

Copy link
@harshavardhana

harshavardhana Apr 13, 2017

Member

We shouldn't use CGO based dependencies in Minio server. You should implement something Go native. @ptribble

This comment has been minimized.

Copy link
@harshavardhana

harshavardhana Apr 14, 2017

Member

Generally as a practice we would like to avoid this, because this causes requirements of cgo. Minio is generally built with CGO_ENABLED=0 and we want to keep it that way for simplicity and avoid depending on gcc + libc altogether.

There might be some limitations on how much Solaris is supported but currently by Go - you can see an example on how it should be implemented here

https://github.com/golang/sys/blob/master/unix/syscall_solaris.go#L706

This comment has been minimized.

Copy link
@harshavardhana

harshavardhana Apr 14, 2017

Member

Another way is simply return error as not implemented and avoid this altogether. This package is used only by caching layer which is enabled during erasure or distributed erasure minio server setup. For regular fs setups this is not needed

We can keep this as a FIXME and address this in subsequent time. @ptribble

@@ -0,0 +1,41 @@
/*
* Minio Cloud Storage, (C) 2016,2017 Minio, Inc.

This comment has been minimized.

Copy link
@balamurugana

balamurugana Apr 14, 2017

Member

New file needs to have 2017 only.

#include <unistd.h>
*/
import (
"C"

This comment has been minimized.

Copy link
@harshavardhana

harshavardhana Apr 14, 2017

Member

Another way is simply return error as not implemented and avoid this altogether. This package is used only by caching layer which is enabled during erasure or distributed erasure minio server setup. For regular fs setups this is not needed

We can keep this as a FIXME and address this in subsequent time. @ptribble

@ptribble
Copy link
Contributor Author

ptribble commented Apr 14, 2017

Thanks for the comments.

OK, let me think about this. I haven't yet found a way to do this without cgo, but I'll dig deeper.

(On solaris, things are a bit different. You always have to have libc, because that [not syscall] is the interface to the system.)

@harshavardhana
Copy link
Member

harshavardhana commented Apr 15, 2017

Thanks for the comments.

OK, let me think about this. I haven't yet found a way to do this without cgo, but I'll dig deeper.

(On solaris, things are a bit different. You always have to have libc, because that [not syscall] is the interface to the system.)

I would suggest we do just return return 0, errors.New("Not implemented") for now so we can get this built properly and work on proper version using x/sys/unix at a later stage? @ptribble

@harshavardhana
Copy link
Member

harshavardhana commented Apr 18, 2017

@ptribble are you still working on this PR? were you able to look at my suggestion above?

@deekoder
Copy link
Contributor

deekoder commented Apr 19, 2017

@ptribble I suggest we close this PR and you can send us a new one when you can without the cgo dependency. Would you be in agreement with this proposal?

@codecov-io
Copy link

codecov-io commented Apr 20, 2017

Codecov Report

Merging #4115 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4115      +/-   ##
==========================================
- Coverage    68.3%   68.25%   -0.05%     
==========================================
  Files         174      174              
  Lines       24181    24207      +26     
==========================================
+ Hits        16516    16523       +7     
- Misses       6555     6574      +19     
  Partials     1110     1110
Impacted Files Coverage Δ
pkg/sys/rlimit-file_nix.go 100% <ø> (ø) ⬆️
pkg/sys/rlimit-memory_nix.go 100% <ø> (ø) ⬆️
cmd/admin-rpc-server.go 40.9% <0%> (-8.55%) ⬇️
cmd/fs-v1-helpers.go 59.02% <0%> (-1.74%) ⬇️
cmd/admin-handlers.go 57.04% <0%> (-0.06%) ⬇️
pkg/madmin/info-commands.go 0% <0%> (ø) ⬆️
cmd/http-stats.go 100% <0%> (ø) ⬆️
cmd/fs-v1-background-append.go 78.52% <0%> (ø) ⬆️
cmd/server-mux.go 84.85% <0%> (+0.05%) ⬆️
cmd/admin-rpc-client.go 44.27% <0%> (+1.95%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1d7780...77b5ffe. Read the comment docs.

@ptribble
Copy link
Contributor Author

ptribble commented Apr 20, 2017

Sorry about the delay; holiday weekend.

I've updated the PR with a stub implementation in stats_solaris.go, and corrected the date.

The build completes, the make checks are clean, and minio server has been tested in fs mode successfully.

I agree, we can defer a full implementation until later.

@harshavardhana
Copy link
Member

harshavardhana commented Apr 20, 2017

I've updated the PR with a stub implementation in stats_solaris.go, and corrected the date.

Thanks @ptribble - taking a look.

Copy link
Member

balamurugana left a comment

solaris cross build fails.

$ GOOS=solaris go build 
vendor/github.com/mattn/go-isatty/isatty_solaris.go:7:2: cannot find package "golang.org/x/sys/unix" in any of:
	/home/bala/golang/gopath/src/github.com/minio/minio/vendor/golang.org/x/sys/unix (vendor tree)
	/home/bala/golang/goroot/src/golang.org/x/sys/unix (from $GOROOT)
	/home/bala/golang/gopath/src/golang.org/x/sys/unix (from $GOPATH)

my go version

$ go version
go version go1.7.5 linux/amd64
@harshavardhana
Copy link
Member

harshavardhana commented Apr 21, 2017

$ GOOS=solaris go build

I know the problem.. it is perhaps because we have failed to vendorize golang.org/x/sys/unix

@harshavardhana
Copy link
Member

harshavardhana commented Apr 21, 2017

@ptribble can you merge this PR in here #4161 - this would completely fix all the build issues on Solaris.

@balamurugana balamurugana mentioned this pull request Apr 21, 2017
3 of 8 tasks complete
@ptribble
Copy link
Contributor Author

ptribble commented Apr 21, 2017

Sure, I can do that. May be a couple of day before I can get to it.

@ptribble
Copy link
Contributor Author

ptribble commented Apr 22, 2017

Ok, I've merged 4161, and also fixed up checkdeps.sh for Solaris (which appears to immediately cause the make style build to fail).

@harshavardhana
Copy link
Member

harshavardhana commented Apr 22, 2017

Ok, I've merged 4161, and also fixed up checkdeps.sh for Solaris (which appears to immediately cause the make style build to fail).

Thanks a lot @ptribble

@@ -25,6 +25,11 @@ _init() {
OSX_VERSION="10.8"
KNAME=$(uname -s)
ARCH=$(uname -m)

This comment has been minimized.

Copy link
@balamurugana

balamurugana Apr 23, 2017

Member

does uname -m exist in Solaris? does isainfo -k provide different value over uname -m?

This comment has been minimized.

Copy link
@ptribble

ptribble Apr 23, 2017

Author Contributor

On Solaris, uname -m returns the type of processor. However, Solaris is dual 32/64-bit so all that uname -m tells you is that you're running on Intel (as opposed to sparc or whatever) - it doesn't tell you whether you're 32 or 64-bit. The isainfo command tells you which instruction set architectures are available; isainfo -k tells you which variant the kernel is running in.

See below for the output on my system:

% uname -m
i86pc
% isainfo
amd64 i386
% isainfo -k
amd64

The checkdeps.sh script is looking for what isainfo -k gives on a Solaris system.

This comment has been minimized.

Copy link
@balamurugana

balamurugana Apr 23, 2017

Member

@ptribble got it. thanks.

@harshavardhana harshavardhana merged commit 2b96d9f into minio:master Apr 23, 2017
4 of 5 checks passed
4 of 5 checks passed
codecov/project 68.25% (-0.05%) compared to f1d7780
Details
codecov/patch Coverage not affected when comparing f1d7780...77b5ffe
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!
@harshavardhana
Copy link
Member

harshavardhana commented Apr 23, 2017

Thanks for the efforts here @ptribble

@ptribble ptribble deleted the ptribble:enable-solaris-build branch Apr 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.