build: a number of fixes on Makefile and nix-shell #360

tiagolobocastro · 2025-01-05T23:33:53Z

What this PR does?:
Ensure the namespace is always set correctly to openebs by default.
Set the image pull policy properly (wrong var was used...)

The non-buildx Dockerfile copies a binary built outside of the docker
build, which may be broken if the dependencies are not met.
This happens on nix-shell as the deps exist on /nix/store.
Get around this by building static binaries in the nix-shell.

Pin a few packages version which are not compatible on latest.
Perhaps all of them should be pinned!
Remove a few uses of sudo which should not be required?
Add a few packages missing from nix-shell
Allow make clean to work properly
Don't bootstrap by default, though this may be discussed further.
Setup goenv env variables on github actions workflows
Add gitignore for the coverage data

Does this PR require any upgrade changes?:

If the changes in this PR are manually verified, list down the scenarios covered::

Any additional information for your reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs

Checklist:

Fixes #
PR Title follows the convention of <type>(<scope>): <subject>
Has the change log section been updated?
Commit has unit tests
Commit has integration tests
(Optional) Are upgrade changes included in this PR? If not, mention the issue/PR to track:
(Optional) If documentation changes are required, which issue on https://github.com/openebs/openebs-docs is used to track them:

shell.nix

tiagolobocastro · 2025-01-05T23:59:21Z

Ah looks like CI failed because GOPATH is not set or empty. Is this expected?
CC @dsharma-dc @Abhinandan-Purkait

tiagolobocastro · 2025-01-06T00:58:48Z

I'm also a bit confused with the bdd-tests on PR, it seems like it's testing against develop rather than the code built in this PR or am I'm missing something?

mhkarimi1383 · 2025-01-06T05:39:24Z

Ah looks like CI failed because GOPATH is not set or empty. Is this expected?
CC @dsharma-dc @Abhinandan-Purkait

I think that's because HOME env inheritance is removed

mhkarimi1383 · 2025-01-06T05:41:35Z

Can you make builds static?
Because docker builds are not working in Nix and some OSes (Because of shared C libs)

See
#335 (comment)

Makefile

Abhinandan-Purkait · 2025-01-06T06:20:01Z

Ah looks like CI failed because GOPATH is not set or empty. Is this expected? CC @dsharma-dc @Abhinandan-Purkait

The setup go action step sets it to home/runner/go

Abhinandan-Purkait · 2025-01-06T06:22:09Z

I'm also a bit confused with the bdd-tests on PR, it seems like it's testing against develop rather than the code built in this PR or am I'm missing something?

Which bit are you talking about? It checkout the PR code and build image for it and then uses it from the built image locally?

tiagolobocastro · 2025-01-06T11:07:00Z

Ah looks like CI failed because GOPATH is not set or empty. Is this expected?
CC @dsharma-dc @Abhinandan-Purkait

I think that's because HOME env inheritance is removed

No: https://github.com/openebs/lvm-localpv/actions/runs/12331398437/job/34417991555#step:7:34

Ah I wanted to ask you, why is this needed? In my case it's always there, even without this line.

tiagolobocastro · 2025-01-06T12:30:23Z

Ah looks like CI failed because GOPATH is not set or empty. Is this expected? CC @dsharma-dc @Abhinandan-Purkait

The setup go action step sets it to home/runner/go

Fixed by taking go env, PTAL

tiagolobocastro · 2025-01-06T15:39:41Z

I'm also a bit confused with the bdd-tests on PR, it seems like it's testing against develop rather than the code built in this PR or am I'm missing something?

Which bit are you talking about? It checkout the PR code and build image for it and then uses it from the built image locally?

Discussed offline, I will update here...
So on GHA minikube is setup with driver none, and thus the images are picked up from the host this way.. this is confusing and should be documented.
Also this makes local testing on custom cluster not very straightforward with current script

tiagolobocastro · 2025-01-06T20:13:21Z

Can you make builds static? Because docker builds are not working in Nix and some OSes (Because of shared C libs)

See #335 (comment)

The problem here is that the dockerfile is simply copying the binary to the container. On non-nix binaries this might work though it might also fail at runtime potentially if the OS libs are not compatible with the container libs.

On the other hand the buildx is building inside the docker build, which is a bit better, though it seems that the same problem may arise from what I can tell looking at it?

I'll make use of musl-gcc on the nix-shell and make build static just for the nix-shell for now? Or shall we make it static for docker buildx as well? CC @Abhinandan-Purkait @dsharma-dc

mhkarimi1383 · 2025-01-06T21:29:11Z

@tiagolobocastro

Yes, I think RHEL Based users may face the same problem, NixOS is a big example since it's so different that other distributions in terms of paths, etc.

I think we can also change our flow to build directly through the Dockerfile

(For example if we are running our runtime in debian we can also use golang debian based image)

mhkarimi1383 · 2025-01-06T21:34:20Z

With static builds we could also publish binary builds later on (I know the usage for that is rare, but we can do it too)

github-actions

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

shellcheck

📝 [shellcheck] _{reported by reviewdog 🐶}
Double quote to prevent globbing and word splitting. SC2086

lvm-localpv/buildscripts/build.sh

Line 100 in bc482b4

if [ $GOOS = "windows" ]; then

📝 [shellcheck] _{reported by reviewdog 🐶}
Double quote to prevent globbing and word splitting. SC2086

lvm-localpv/buildscripts/build.sh

Line 118 in bc482b4

GOPATH="$(cygpath $GOPATH)"

⚠️ [shellcheck] _{reported by reviewdog 🐶}
Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a. SC2206

lvm-localpv/buildscripts/build.sh

Line 122 in bc482b4

IFS=: MAIN_GOPATH=($GOPATH)

⚠️ [shellcheck] _{reported by reviewdog 🐶}
Expanding an array without an index only gives the first element. SC2128

lvm-localpv/buildscripts/build.sh

Line 126 in bc482b4

mkdir -p ${MAIN_GOPATH}/bin/

📝 [shellcheck] _{reported by reviewdog 🐶}
Double quote to prevent globbing and word splitting. SC2086

lvm-localpv/buildscripts/build.sh

Line 126 in bc482b4

mkdir -p ${MAIN_GOPATH}/bin/

⚠️ [shellcheck] _{reported by reviewdog 🐶}
For loops over find output are fragile. Use find -exec or a while read loop. SC2044

lvm-localpv/buildscripts/build.sh

Line 130 in bc482b4

for F in $(find ${DEV_PLATFORM} -mindepth 1 -maxdepth 1 -type f); do

📝 [shellcheck] _{reported by reviewdog 🐶}
Double quote to prevent globbing and word splitting. SC2086

lvm-localpv/buildscripts/build.sh

Line 130 in bc482b4

for F in $(find ${DEV_PLATFORM} -mindepth 1 -maxdepth 1 -type f); do

📝 [shellcheck] _{reported by reviewdog 🐶}
Double quote to prevent globbing and word splitting. SC2086

lvm-localpv/buildscripts/build.sh

Line 131 in bc482b4

cp ${F} bin/${PNAME}/

📝 [shellcheck] _{reported by reviewdog 🐶}
Double quote to prevent globbing and word splitting. SC2086

lvm-localpv/buildscripts/build.sh

Line 131 in bc482b4

cp ${F} bin/${PNAME}/

📝 [shellcheck] _{reported by reviewdog 🐶}
Double quote to prevent globbing and word splitting. SC2086

lvm-localpv/buildscripts/build.sh

Line 132 in bc482b4

cp ${F} ${MAIN_GOPATH}/bin/

⚠️ [shellcheck] _{reported by reviewdog 🐶}
Expanding an array without an index only gives the first element. SC2128

lvm-localpv/buildscripts/build.sh

Line 132 in bc482b4

cp ${F} ${MAIN_GOPATH}/bin/

📝 [shellcheck] _{reported by reviewdog 🐶}
Double quote to prevent globbing and word splitting. SC2086

lvm-localpv/buildscripts/build.sh

Line 132 in bc482b4

cp ${F} ${MAIN_GOPATH}/bin/

📝 [shellcheck] _{reported by reviewdog 🐶}
Double quote to prevent globbing and word splitting. SC2086

lvm-localpv/buildscripts/build.sh

Line 138 in bc482b4

ls -hl bin/${PNAME}/

tiagolobocastro · 2025-01-07T00:30:29Z

A ton of shellcheck warnings were given because I modified the shell file, this is not great to be honest, now it's messed up this PR with a ton of comments.
It would be better to get these in the action, not as comments... -.-

dsharma-dc · 2025-01-07T05:00:48Z

Can you make builds static? Because docker builds are not working in Nix and some OSes (Because of shared C libs)
See #335 (comment)

The problem here is that the dockerfile is simply copying the binary to the container. On non-nix binaries this might work though it might also fail at runtime potentially if the OS libs are not compatible with the container libs.

On the other hand the buildx is building inside the docker build, which is a bit better, though it seems that the same problem may arise from what I can tell looking at it?

I'll make use of musl-gcc on the nix-shell and make build static just for the nix-shell for now? Or shall we make it static for docker buildx as well? CC @Abhinandan-Purkait @dsharma-dc

If we want to make static for nix as well as docker build, then better do that across all localpv repos if they are also following same?

Abhinandan-Purkait · 2025-01-07T05:46:54Z

Can you make builds static? Because docker builds are not working in Nix and some OSes (Because of shared C libs)
See #335 (comment)

The problem here is that the dockerfile is simply copying the binary to the container. On non-nix binaries this might work though it might also fail at runtime potentially if the OS libs are not compatible with the container libs.

On the other hand the buildx is building inside the docker build, which is a bit better, though it seems that the same problem may arise from what I can tell looking at it?

I'll make use of musl-gcc on the nix-shell and make build static just for the nix-shell for now? Or shall we make it static for docker buildx as well? CC @Abhinandan-Purkait @dsharma-dc

Yeah, I think that's a better way in general. If we can do it across all repos.

tiagolobocastro · 2025-01-07T11:02:01Z

@Abhinandan-Purkait @dsharma-dc @mhkarimi1383

Ok, it is done, please give this around review

A lot of issues on the existing code but seems they are only warned if the file is modified... Signed-off-by: Tiago Castro <[email protected]>

Pin a few packages version which are not compatible on latest. Perhaps all of them should be pinned! Remove a few uses of sudo which should not be required? Add a few packages missing from nix-shell Allow make clean to work properly Don't bootstrap by default, though this may be discussed further. Setup goenv env variables on github actions workflows Add gitignore for the coverage data Signed-off-by: Tiago Castro <[email protected]>

The non-buildx Dockerfile copies a binary built outside of the docker build, which may be broken if the dependencies are not met. This happens on nix-shell as the deps exist on /nix/store. Get around this by building static binaries in the nix-shell. We now depend on golang-alpine image and install musl-dev as well. Signed-off-by: Tiago Castro <[email protected]>

Ensure the namespace is always set correctly to openebs by default. Set the image pull policy properly (wrong var was used...) Signed-off-by: Tiago Castro <[email protected]>

gingko lvm_utils creates some loop devices and vgs, which are not cleanup in some cases (eg: test failures). furthermore, even between test cases, leftovers are causing subsequent tests to fails, but that's another matter that needs fixing. also ensure make clean cleans these up Signed-off-by: Tiago Castro <[email protected]>

Adds lvm2 and jq Also ensure sudo works in a pure shell Signed-off-by: Tiago Castro <[email protected]>

mhkarimi1383 · 2025-01-08T09:32:22Z

I have tested the build locally and it was working fine

mhkarimi1383 · 2025-01-06T05:38:08Z

shell.nix

  shellHook = ''
-    export HOME=${builtins.getEnv "HOME"}


HOME env variable should be inherited explicitly when we are using this in GitHub Actions

If GitHub Actions are working fine we can ignore this :)

tiagolobocastro requested review from Abhinandan-Purkait, niladrih, abhilashshetty04 and dsharma-dc January 5, 2025 23:33

tiagolobocastro commented Jan 5, 2025

View reviewed changes

shell.nix Show resolved Hide resolved

Abhinandan-Purkait reviewed Jan 6, 2025

View reviewed changes

Makefile Outdated Show resolved Hide resolved

tiagolobocastro force-pushed the nix-fixes branch 2 times, most recently from 7fcd6bb to 226c3d2 Compare January 6, 2025 11:28

tiagolobocastro force-pushed the nix-fixes branch 2 times, most recently from 3bcaf59 to 5a38965 Compare January 6, 2025 20:08

tiagolobocastro force-pushed the nix-fixes branch from 5a38965 to bc482b4 Compare January 6, 2025 23:52

github-actions bot reviewed Jan 6, 2025

View reviewed changes

openebs deleted a comment from github-actions bot Jan 7, 2025

tiagolobocastro force-pushed the nix-fixes branch 2 times, most recently from a2f4d8c to 723bdfd Compare January 7, 2025 11:00

tiagolobocastro force-pushed the nix-fixes branch 2 times, most recently from 37f58a3 to 9869a71 Compare January 7, 2025 12:56

tiagolobocastro added 4 commits January 7, 2025 16:03

refactor: deal with shellcheck

119a332

A lot of issues on the existing code but seems they are only warned if the file is modified... Signed-off-by: Tiago Castro <[email protected]>

test: default namespace and set image pull policy

ead994c

Ensure the namespace is always set correctly to openebs by default. Set the image pull policy properly (wrong var was used...) Signed-off-by: Tiago Castro <[email protected]>

tiagolobocastro force-pushed the nix-fixes branch from 9869a71 to 63547cc Compare January 7, 2025 16:10

tiagolobocastro added 2 commits January 7, 2025 16:13

build(nix-shell): add missing lvm packages to pure nix-shell

d423456

Adds lvm2 and jq Also ensure sudo works in a pure shell Signed-off-by: Tiago Castro <[email protected]>

tiagolobocastro force-pushed the nix-fixes branch from 63547cc to d423456 Compare January 7, 2025 16:13

dsharma-dc approved these changes Jan 8, 2025

View reviewed changes

Abhinandan-Purkait approved these changes Jan 8, 2025

View reviewed changes

mhkarimi1383 approved these changes Jan 8, 2025

View reviewed changes

tiagolobocastro merged commit 9a95c31 into develop Jan 8, 2025
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: a number of fixes on Makefile and nix-shell #360

build: a number of fixes on Makefile and nix-shell #360

tiagolobocastro commented Jan 5, 2025 •

edited

Loading

tiagolobocastro commented Jan 5, 2025

tiagolobocastro commented Jan 6, 2025

mhkarimi1383 commented Jan 6, 2025

mhkarimi1383 commented Jan 6, 2025

Abhinandan-Purkait commented Jan 6, 2025

Abhinandan-Purkait commented Jan 6, 2025

tiagolobocastro commented Jan 6, 2025

tiagolobocastro commented Jan 6, 2025

tiagolobocastro commented Jan 6, 2025

tiagolobocastro commented Jan 6, 2025

mhkarimi1383 commented Jan 6, 2025

mhkarimi1383 commented Jan 6, 2025

github-actions bot left a comment

tiagolobocastro commented Jan 7, 2025

dsharma-dc commented Jan 7, 2025

Abhinandan-Purkait commented Jan 7, 2025

tiagolobocastro commented Jan 7, 2025

mhkarimi1383 commented Jan 8, 2025

mhkarimi1383 Jan 6, 2025

mhkarimi1383 Jan 8, 2025

build: a number of fixes on Makefile and nix-shell #360

build: a number of fixes on Makefile and nix-shell #360

Conversation

tiagolobocastro commented Jan 5, 2025 • edited Loading

tiagolobocastro commented Jan 5, 2025

tiagolobocastro commented Jan 6, 2025

mhkarimi1383 commented Jan 6, 2025

mhkarimi1383 commented Jan 6, 2025

Abhinandan-Purkait commented Jan 6, 2025

Abhinandan-Purkait commented Jan 6, 2025

tiagolobocastro commented Jan 6, 2025

tiagolobocastro commented Jan 6, 2025

tiagolobocastro commented Jan 6, 2025

tiagolobocastro commented Jan 6, 2025

mhkarimi1383 commented Jan 6, 2025

mhkarimi1383 commented Jan 6, 2025

github-actions bot left a comment

Choose a reason for hiding this comment

tiagolobocastro commented Jan 7, 2025

dsharma-dc commented Jan 7, 2025

Abhinandan-Purkait commented Jan 7, 2025

tiagolobocastro commented Jan 7, 2025

mhkarimi1383 commented Jan 8, 2025

mhkarimi1383 Jan 6, 2025

Choose a reason for hiding this comment

mhkarimi1383 Jan 8, 2025

Choose a reason for hiding this comment

tiagolobocastro commented Jan 5, 2025 •

edited

Loading