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.

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

Breaking Makefile into platform specific files. #1388

Merged
merged 1 commit into from
Aug 22, 2017
Merged

Breaking Makefile into platform specific files. #1388

merged 1 commit into from
Aug 22, 2017

Conversation

kunalkushwaha
Copy link
Contributor

@kunalkushwaha kunalkushwaha commented Aug 18, 2017 •
edited
Loading

Fixes #1270

Signed-off-by: Kunal Kushwaha kushwaha_kunal_v7@lab.ntt.co.jp

@epilatow
Copy link
Contributor

Solaris doesn't support the "-race" test flags option, so that should be removed from Makefile.solaris.

@kunalkushwaha
Copy link
Contributor Author

thanks for pointing out. Updated the file.

estesp
estesp requested changes Aug 18, 2017
View reviewed changes
Copy link
Member

@estesp estesp 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 starting this work @kunalkushwaha; couple minor things noted, otherwise LGTM

Makefile Outdated
TESTFLAGS_RACE= -race
endif

#include platform sepcific makefile
Copy link
Member

Choose a reason for hiding this comment

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

typo: s/sepcific/specific/

Makefile Outdated
endif

#include platform sepcific makefile
include Makefile.${GOOS}
Copy link
Member

Choose a reason for hiding this comment

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

Given we use parenthesis instead of curly brace as a variable delimiter throughout this Makefile, seems like we should use it for consistency here (even though functionally it doesn't matter)

mlaventure
mlaventure reviewed Aug 18, 2017
View reviewed changes
Makefile.darwin
COMMANDS += containerd-shim

# supports go test -race
TESTFLAGS_RACE= -race
Copy link
Contributor

Choose a reason for hiding this comment

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

can we get a newline at the ends of the files? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added newline in each file.

@codecov-io
Copy link

codecov-io commented Aug 21, 2017 •
edited
Loading

Codecov Report

Merging #1388 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1388   +/-   ##
=======================================
  Coverage   40.81%   40.81%           
=======================================
  Files          23       23           
  Lines        2923     2923           
=======================================
  Hits         1193     1193           
  Misses       1452     1452           
  Partials      278      278

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 89da512...1c98958. Read the comment docs.

@kunalkushwaha
Copy link
Contributor Author

updated with changes requested.

mlaventure
mlaventure approved these changes Aug 21, 2017
View reviewed changes
Copy link
Contributor

@mlaventure mlaventure left a comment

Choose a reason for hiding this comment

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

LGTM

/cc @stevvooe

@stevvooe
Copy link
Member

LGTM

This is a good start.

@crosbymichael
Copy link
Member

crosbymichael commented Aug 21, 2017 •
edited
Loading

NOT LGTM

This breaks

make && sudo make install
🇩 bin/ctr
🇩 bin/containerd
🇩 bin/containerd-stress
🇩 bin/containerd-shim
🇩 binaries
Makefile:50: Makefile.$GOOS: No such file or directory
make: *** No rule to make target 'Makefile.$GOOS'.  Stop.

sudo make install should not require Go or anything of the sort, it should just install.

@kunalkushwaha kunalkushwaha force-pushed the makefile branch 3 times, most recently from 792e0af to 808f636 Compare August 22, 2017 03:15
@kunalkushwaha
Copy link
Contributor Author

kunalkushwaha commented Aug 22, 2017 •
edited
Loading

Since, platform specific Makefiles are required in make install , added Makefile.OS, to detect OS without go and still keeps main Makefile simple.

@kunalkushwaha kunalkushwaha force-pushed the makefile branch 2 times, most recently from 67f2fd7 to 1c98958 Compare August 22, 2017 04:52
@kunalkushwaha
Breaking Makefile into platform specific files
1c98958 
Fixed containerd#1270

Signed-off-by: Kunal Kushwaha <kushwaha_kunal_v7@lab.ntt.co.jp>
@estesp
Copy link
Member

estesp commented Aug 22, 2017

Looks like using uname -s works as a replacement for Go specific environment test; I can only test Linux and macOS (Darwin); not sure if we need validation for the others.

@crosbymichael PTAL

@crosbymichael
Copy link
Member

LGTM

@crosbymichael crosbymichael merged commit 0648ec3 into containerd:master Aug 22, 2017
@crosbymichael
Copy link
Member

@kunalkushwaha thanks for taking care of make install

@schomatis schomatis mentioned this pull request Jan 5, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estesp estesp estesp approved these changes

@mlaventure mlaventure mlaventure approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@kunalkushwaha @epilatow @codecov-io @stevvooe @crosbymichael @estesp @mlaventure