-
Notifications
You must be signed in to change notification settings - Fork 110
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
RSDK-4829 arm32 build #2790
RSDK-4829 arm32 build #2790
Changes from all commits
5e226e9
68c7ccb
75e0d4c
6b60670
66d67e6
a83987c
6891482
c935ae7
980ee58
692c153
732e0cd
f5cea48
b11f091
1c1031d
e834d77
56a64d0
e1408d4
310ea32
306b07b
f6b6d75
b585668
b854cc7
e59a913
3ef4ae9
a2cd8af
c6ae496
9773171
ba83c4b
97265ec
2223c25
f4d9db7
8697e1b
38dcec5
d1be329
af4a537
964c87a
971b956
00dd665
ce8f317
bbf5226
1068efa
5fd6175
ede8747
9cc003c
bd8add3
50865ae
bb35054
8e6875e
7e970c0
bb8b997
792c249
69f2dc4
093bb4a
eed357b
023bcaa
4eee745
7f70fc7
4ebc935
bce9cac
6a05461
4e1672e
2725539
6639563
9d33d47
284dabf
6c8f40e
2fcd919
b67228b
89391eb
5b4c961
145dd2a
377ab58
3aeed32
5055cc5
2e728ee
298773a
2085c63
27f8b44
2f12646
6476760
4860eea
178be70
9770865
ceb4343
60f60f3
68aea5c
7fe599a
ff7350a
884bf56
d2a52a7
8cdfadc
1f609e2
809e712
ab16a5f
b8eb020
25f748d
400b6b3
bcaf04f
d38b09b
b112bf0
580049a
53acc87
8aad23c
6115e3f
35830a8
e579526
1358c95
bd1f265
1916e0c
98851a4
ed3c302
82a90b3
16c76fa
045cbfd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
bin/ | ||
.direnv/ | ||
.git/ | ||
**/Dockerfile |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,115 @@ | ||
on: | ||
push: | ||
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't want to be building a new docker image every single push. This needs to be a called workflow, hooked into the proper "top-level" workflows (e.g. pull-request-trusted.yml and/or main.yml) |
||
|
||
jobs: | ||
docker: | ||
runs-on: buildjet-4vcpu-ubuntu-2204-arm | ||
permissions: | ||
packages: write | ||
steps: | ||
- uses: actions/checkout@v3 | ||
- uses: docker/login-action@v2 | ||
with: | ||
username: awinterbuildrate | ||
password: ${{ secrets.DOCKER_PUBLIC_READONLY_PAT }} | ||
- uses: docker/setup-qemu-action@v2 | ||
- uses: docker/setup-buildx-action@v2 | ||
- uses: docker/login-action@v2 | ||
with: | ||
registry: ghcr.io | ||
username: ${{ github.actor }} | ||
password: ${{ secrets.GITHUB_TOKEN }} | ||
- uses: docker/build-push-action@v4 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't appear to be building a 32-bit image. The image currently at ghcr.io/viamrobotics/viam-server:armv7 is actually 64bit arm when I inspect it as well.
|
||
with: | ||
file: etc/packaging/32bit/Dockerfile | ||
tags: ghcr.io/viamrobotics/viam-server:armv7 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name of the docker image probably shouldn't be the same as our binary, but it's fine for the moment. |
||
pull: true | ||
push: true | ||
cache-from: type=registry,ref=ghcr.io/viamrobotics/viam-server:buildcache | ||
cache-to: type=registry,ref=ghcr.io/viamrobotics/viam-server:buildcache | ||
lint: | ||
needs: [docker] | ||
runs-on: buildjet-4vcpu-ubuntu-2204-arm | ||
permissions: | ||
packages: read | ||
steps: | ||
- uses: docker/login-action@v2 | ||
with: | ||
registry: ghcr.io | ||
username: ${{ github.actor }} | ||
password: ${{ secrets.GITHUB_TOKEN }} | ||
Comment on lines
+36
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't believe you need this when using the image (only when publishing.) The docker images should be public. |
||
- uses: actions/checkout@v3 | ||
# note: we're linting on 32-bit because atomicalign analyzer only works on 32-bit architecture | ||
- name: lint | ||
run: docker run --rm -v $PWD/:/rdk ghcr.io/viamrobotics/viam-server:armv7 make lint-32bit | ||
- id: changed-files | ||
uses: tj-actions/changed-files@v39 | ||
with: | ||
# note: this doesn't provide perfect coverage, but it's a compromise between running all the time + never running | ||
since_last_remote_commit: true | ||
- name: vendorlint | ||
if: contains(steps.changed-files.outputs.modified_files, 'go.sum') | ||
run: docker run --rm -v $PWD/:/rdk ghcr.io/viamrobotics/viam-server:armv7 make vendorlint | ||
test: | ||
needs: [docker] | ||
runs-on: buildjet-4vcpu-ubuntu-2204-arm | ||
strategy: | ||
fail-fast: false | ||
matrix: | ||
index: ['0', '1', '2', '3', '4'] | ||
permissions: | ||
packages: read | ||
steps: | ||
- uses: docker/login-action@v2 | ||
with: | ||
registry: ghcr.io | ||
username: ${{ github.actor }} | ||
password: ${{ secrets.GITHUB_TOKEN }} | ||
- uses: actions/checkout@v3 | ||
- name: test | ||
run: | | ||
docker run --rm \ | ||
-v $PWD/:/rdk \ | ||
--env GOFLAGS="-tags=no_tflite,no_pigpio -buildvcs=false" \ | ||
abe-winter marked this conversation as resolved.
Show resolved
Hide resolved
|
||
--env INDEX=${{ matrix.index }} \ | ||
--env NBINS=5 \ | ||
ghcr.io/viamrobotics/viam-server:armv7 \ | ||
make test-go-split | ||
build: | ||
needs: [test, lint] | ||
runs-on: buildjet-4vcpu-ubuntu-2204-arm | ||
permissions: | ||
packages: read | ||
steps: | ||
- uses: docker/login-action@v2 | ||
with: | ||
registry: ghcr.io | ||
username: ${{ github.actor }} | ||
password: ${{ secrets.GITHUB_TOKEN }} | ||
- uses: actions/checkout@v3 | ||
- name: build | ||
run: docker run --rm -v $PWD/:/rdk ghcr.io/viamrobotics/viam-server:armv7 sh -c "git config --global --add safe.directory '*' && go build -v -tags no_pigpio,no_tflite ./web/cmd/server" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm unclear why you're directly calling docker run, rather than using the workflow-native "container" options to run inside the container. |
||
- name: show arch | ||
run: file server | ||
- uses: actions/upload-artifact@v3 | ||
with: | ||
path: server | ||
upload: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this a seperate job? Why not just upload in the job where you created the artifact? |
||
needs: [build] | ||
if: github.ref_name == 'main' | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/download-artifact@v3 | ||
- name: rename | ||
run: mv artifact/server viam-server-latest-armv7l | ||
- name: Authorize GCP Upload | ||
uses: google-github-actions/auth@v1 | ||
with: | ||
credentials_json: '${{ secrets.GCP_CREDENTIALS }}' | ||
- name: upload | ||
uses: google-github-actions/[email protected] | ||
with: | ||
headers: "cache-control: no-cache" | ||
path: viam-server-latest-armv7l | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't believe this is a distributable binary. There's a whole process to do a (mostly) statically linked binary and/or appimages. This appears to just be raw binary, and with the current docker image, a 64-bit one at that. |
||
destination: 'packages.viam.com/apps/viam-server/' | ||
parent: false |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,6 +58,14 @@ lint-go: tool-install | |
export pkgs="`go list -f '{{.Dir}}' ./... | grep -v /proto/`" && echo "$$pkgs" | xargs go vet -vettool=$(TOOL_BIN)/combined | ||
GOGC=50 $(TOOL_BIN)/golangci-lint run -v --fix --config=./etc/.golangci.yaml | ||
|
||
lint-32bit: tool-install | ||
$(TOOL_BIN)/golangci-lint run -v --build-tags no_tflite,no_pigpio --tests=false --disable-all --enable staticcheck --timeout 30m | ||
|
||
vendorlint: tool-install | ||
go mod vendor | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an expensive (bandwidth/time) operation. It also leaves the vendor directory permanently which breaks future builds for regular use. To be specific, |
||
# parallelism is 1 for now because golangci-lint detects parallel runs and complains | ||
etc/vendorlint.py all --linter $(abspath $(TOOL_BIN)/golangci-lint) --parallel 1 --out vendorlint.json | ||
|
||
lint-web: check-web | ||
npm run lint --prefix web/frontend | ||
|
||
|
@@ -74,6 +82,10 @@ test-no-race: test-go-no-race test-web | |
test-go: tool-install | ||
PATH=$(PATH_WITH_TOOLS) ./etc/test.sh race | ||
|
||
test-go-split: | ||
# run one shard of the test suite using splitter.py | ||
go list -json ./... | etc/splitter.py $(INDEX) --nbins $(NBINS) --command "go test" --fail-empty | ||
|
||
test-go-no-race: tool-install | ||
PATH=$(PATH_WITH_TOOLS) ./etc/test.sh | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
//go:build linux && (arm64 || arm) | ||
//go:build linux && (arm64 || arm) && !no_pigpio | ||
|
||
package piimpl | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
//go:build linux && (arm64 || arm) | ||
//go:build linux && (arm64 || arm) && !no_pigpio | ||
|
||
package piimpl | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
//go:build linux && (arm64 || arm) | ||
//go:build linux && (arm64 || arm) && !no_pigpio | ||
|
||
package piimpl | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
//go:build !no_pigpio | ||
#pragma once | ||
|
||
// interruptCallback calls through to the go linked interrupt callback. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
//go:build linux && (arm64 || arm) | ||
//go:build linux && (arm64 || arm) && !no_pigpio | ||
|
||
package piimpl | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
//go:build linux && (arm64 || arm) | ||
//go:build linux && (arm64 || arm) && !no_pigpio | ||
|
||
package piimpl | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
//go:build linux && (arm64 || arm) | ||
//go:build linux && (arm64 || arm) && !no_pigpio | ||
|
||
package piimpl | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
//go:build !(linux && (arm64 || arm)) | ||
//go:build !(linux && (arm64 || arm) && !no_pigpio) | ||
|
||
package pi | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
//go:build linux && (arm64 || arm) | ||
//go:build linux && (arm64 || arm) && !no_pigpio | ||
|
||
package pi | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
//go:build !no_tflite | ||
|
||
package transformpipeline | ||
|
||
import ( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
//go:build !no_tflite | ||
|
||
package transformpipeline | ||
|
||
import ( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,9 +30,6 @@ func init() { | |
// Encoder keeps track of a motor position using a rotary incremental encoder. | ||
type Encoder struct { | ||
resource.Named | ||
|
||
positionType encoder.PositionType | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For posterity:
Offline discussion: |
||
|
||
mu sync.Mutex | ||
A, B board.DigitalInterrupt | ||
// The position is pRaw with the least significant bit chopped off. | ||
|
@@ -53,6 +50,7 @@ type Encoder struct { | |
cancelCtx context.Context | ||
cancelFunc func() | ||
activeBackgroundWorkers sync.WaitGroup | ||
positionType encoder.PositionType | ||
} | ||
|
||
// Pins describes the configuration of Pins for a quadrature encoder. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -173,6 +173,7 @@ func newEncodedMotor( | |
|
||
// EncodedMotor is a motor that utilizes an encoder to track its position. | ||
type EncodedMotor struct { | ||
rpmMonitorCalls int64 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Offline discussion: consider changing this to |
||
resource.Named | ||
resource.AlwaysRebuild | ||
|
||
|
@@ -195,12 +196,11 @@ type EncodedMotor struct { | |
flip int64 // defaults to 1, becomes -1 if the motor config has a true DirectionFLip bool | ||
ticksPerRotation int64 | ||
|
||
rpmMonitorCalls int64 | ||
logger golog.Logger | ||
cancelCtx context.Context | ||
cancel func() | ||
loop *control.Loop | ||
opMgr *operation.SingleOperationManager | ||
logger golog.Logger | ||
cancelCtx context.Context | ||
cancel func() | ||
loop *control.Loop | ||
opMgr *operation.SingleOperationManager | ||
} | ||
|
||
// EncodedMotorState is the core, non-statistical state for the motor. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
FROM debian:bullseye | ||
|
||
RUN apt-get update && apt-get install sudo | ||
RUN adduser --disabled-password runner | ||
RUN echo '%sudo ALL=(ALL) NOPASSWD:ALL' >> /etc/sudoers | ||
RUN adduser runner sudo | ||
USER runner | ||
WORKDIR /build | ||
COPY etc/setup.sh . | ||
RUN ./setup.sh | ||
RUN sudo apt-get install -qqy wget xz-utils file vim pkg-config | ||
WORKDIR /rdk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these things generated by our
make
commands that a user/github action is expected to have created now? Or were these artifacts of your personal workflow?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: this was already blocked in our .gitignore -- I added to .dockerignore to make my docker builds start faster
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are in theory nice-to-haves for anyone doing local docker iterations, but are not critical and I can remove if you want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me, though I'm rather unfamiliar. I'll defer to @Otterverse if there's any concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see .direnv anywhere in .gitignore, so not sure where that came from. Ignoring .git is good though, saves some code size for most builds. There's other stuff in .gitignore that could be here too... especially node_modules and .artifact I think.
Also, why ignore **/Dockerfile ?