Skip to content
This repository has been archived by the owner on Jun 28, 2024. It is now read-only.

travis: add linux-ppc64le and osx #692

Merged
merged 3 commits into from
Sep 7, 2018

Conversation

chavafg
Copy link
Contributor

@chavafg chavafg commented Aug 31, 2018

Run statick checks on linux-ppc64le and osx.

Fixes: #691.

Signed-off-by: Salvador Fuentes salvador.fuentes@intel.com

@chavafg
Copy link
Contributor Author

chavafg commented Aug 31, 2018

getting this error:

.ci/static-checks.sh: line 24: typeset: -A: invalid option

but that one should not be a problem if installing latest available bash, right?
any idea @jodh-intel @raravena80?

@raravena80
Copy link
Member

@chavafg right. But brew installs it under /usr/local/bin/bash and not /usr/bin/bash (which is the system bash)

@marcov
Copy link
Contributor

marcov commented Aug 31, 2018

@raravena80 right, so @chavafg should probably add something like:

before_script:
  - if [[ "$TRAVIS_OS_NAME" == "osx" ]]; then export PATH=/usr/local/bin:$PATH; fi

@chavafg
Copy link
Contributor Author

chavafg commented Aug 31, 2018

ohh right, thank you both, let me add that line.

@chavafg chavafg force-pushed the topic/travis-add-os branch from b91ce14 to b401aa4 Compare August 31, 2018 17:57
@marcov
Copy link
Contributor

marcov commented Aug 31, 2018

Also you need to change the shebang in .ci/static_checks.sh with #!/usr/bin/env bash

@chavafg chavafg force-pushed the topic/travis-add-os branch from b401aa4 to 7b66672 Compare August 31, 2018 18:26
@jodh-intel
Copy link
Contributor

jodh-intel commented Sep 3, 2018

Thanks @chavafg!

lgtm

/cc @raravena80.

Approved with PullApprove

@chavafg chavafg force-pushed the topic/travis-add-os branch 5 times, most recently from 72e29e6 to 5c21bbf Compare September 3, 2018 23:14
@chavafg
Copy link
Contributor Author

chavafg commented Sep 3, 2018

Seems like SIGSTKFLT and SIGPWR are not available in Darwin, any idea of how can I skip those if running on Darwin, but use them if running on Linux?

integration/docker/kill_test.go:135:14:warning: SIGSTKFLT not declared by package syscall (staticcheck)
integration/docker/kill_test.go:135:14:warning: unused variable or constant SIGSTKFLT not declared by package syscall (varcheck)
integration/docker/kill_test.go:149:14:warning: unused variable or constant SIGPWR not declared by package syscall (varcheck)
integration/docker/kill_test.go:135:14:warning: unused struct field SIGSTKFLT not declared by package syscall (structcheck)
integration/docker/kill_test.go:149:14:warning: unused struct field SIGPWR not declared by package syscall (structcheck)
integration/docker/kill_test.go:135:14:warning: SIGSTKFLT not declared by package syscall (unused)

@caoruidong
Copy link
Member

Split them into two files with +build tags?

@marcov
Copy link
Contributor

marcov commented Sep 4, 2018

@chavafg you can safely remove SIGSTKFLT (it's unused on Linux and not existing on darwin).

About SIGPWR, is not a POSIX signal and maybe that can be removed too. From signal(7):

 SIGSTKFLT    -,16,-     Term    Stack fault on coprocessor (unused) 
       SIGPWR (which is not specified in POSIX.1-2001) is typically ignored by default on those other  UNIX  systems  
       where it appears

INSTALL_FLAGS :=

ifeq ($(detected_OS),Linux)
INSTALL_FLAGS := -D
Copy link
Contributor

Choose a reason for hiding this comment

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

To keep things cleaner you can avoid checking OS version here and just run:

install: $(TARGET)
	install -d $(shell dirname $(DESTTARGET))
	install $(TARGET) $(DESTTARGET)

@jodh-intel
Copy link
Contributor

Agreed - we've already removed SIGSTKFLT from the shim and proxy:

We still have SIGPWR in the runtime and the default behaviour appears to be "terminate". To handle that for Darwin, you could adopt the approach we're using in the runtime which:

  • Creates architecture-specific .go files.
  • Creates variables and functions prefixed with arch*. Those variables and functions then either provide a custom (architecture-specific) implementation, or they are set to a function called generic* to provide the default behaviour.

Specifically, I think you could:

  • Move the signal and bool values in the withSignal() calls into a var genericSignalMap map[syscall.Signal]bool, but don't add SIGPWR to the map.
  • Create integration/docker/kill_linux_test.go which will:
    • Create a var archSignalMap = genericSignalMap.
    • Add a syscall.SIGPWR entry to the map (maybe in a func init() function).
    • Call the common Describe() function in integration/docker/kill_test.go.
  • Create integration/docker/kill_darwin_test.go which will:
    • Create a var archSignalMap = genericSignalMap.
    • Call the common Describe() function in integration/docker/kill_test.go.
  • Rework the Describe() function to iterate the archSignalMap, calling WithSignal() on the values.

Run static checks on linux-ppc64le and osx.

Fixes: kata-containers#691.

Signed-off-by: Salvador Fuentes <salvador.fuentes@intel.com>
`install -D` is not supported on Darwin, so we need
to modify how kata-log-parser is installed.

Signed-off-by: Salvador Fuentes <salvador.fuentes@intel.com>
SIGPWR is not supported on Darwin. This patch
adds new kill_linux_test.go to use SIGPWR and
kill_darwin_test.go that does not use SIGPWR.

In addition SIGSTKFLT was removed from the tests.

Signed-off-by: Salvador Fuentes <salvador.fuentes@intel.com>
@chavafg chavafg force-pushed the topic/travis-add-os branch from 5c21bbf to 99e348d Compare September 7, 2018 15:22
@chavafg
Copy link
Contributor Author

chavafg commented Sep 7, 2018

@marcov, @jodh-intel thanks for your suggestions, I already applied the changes, PTAL.

@jodh-intel
Copy link
Contributor

Thanks @chavafg!

lgtm

@GabyCT
Copy link
Contributor

GabyCT commented Sep 7, 2018

lgtm

Approved with PullApprove

@GabyCT GabyCT merged commit f8ebf60 into kata-containers:master Sep 7, 2018
@egernst egernst removed the review label Sep 7, 2018
@chavafg chavafg deleted the topic/travis-add-os branch September 10, 2018 14:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants