Fix get-deps tool to handle non LinuxKit containers #4168

rucoder · 2024-08-27T05:44:46Z

The tool assumed that all lfedge/... docker images are LinuxKit images and there must be pkg/ directory which it not true for e.g. eve-rust container lfedge/eve-rust:1.80.1 Improve version parsing algorithm

@jsfakian I think you also need this PR

rucoder · 2024-08-27T05:53:48Z

The tool assumed that all lfedge/... docker images are LinuxKit images and there must be pkg/ directory which it not true for e.g. eve-rust container lfedge/eve-rust:1.80.1 Improve version parsing algorithm

@jsfakian I think you also need this PR

I realized that it is easier to check that the pkg/<image> directory exists...

deitch · 2024-08-27T06:26:14Z

tools/get-deps/main.go

 	for _, s := range deps {
 		// We are just interested on packages from lfegde (those that we
 		// published)
 		if reLF.MatchString(s) {
+			matches := rePkg.FindStringSubmatch(s)


I am not sure I get what this is doing. I think that it is:

before: if it matched lfedge/eve-:.* then assume that it is pkg/<everything between eve-and tag :>

after: if it matched lfedge/eve-:<sha256> then assume that it is pkg/<everything between eve-and tag :>

Is that it?

If so:

Do you mind adding a README to this? You actually figured out how this works, it is good to capture it. Something that just describes what this does (in a sentence or two) and more importantly, by what logic it filters (the important part).

Is that solid enough? I can see other things following that path that do not come out of here.

Should we add a check for the path? Meaning, instead of (or in addition to), "if it is lfedge/eve-.*:<sha256>", maybe also check if pkg/$1 exists, and, if not, clearly it doesn't belong as part of get-deps, so skip it?

3. Should we add a check for the path? Meaning, instead of (or in addition to), "if it is lfedge/eve-.*:<sha256>", maybe also check if pkg/$1 exists, and, if not, clearly it doesn't belong as part of get-deps, so skip it?

yes, this is what I wrote above. Maybe the easiest way.

Do you mind adding a README to this

I would, but not now. I fixed this one because it was a show stopper for my 'eve visibility' task

Aw, no big deal. Two paragraphs. You know it never will get done otherwise. Who is going to take time to write it later?

yes, this is what I wrote above. Maybe the easiest way

Go for it.

christoph-zededa · 2024-08-27T08:21:11Z

tools/get-deps/main_test.go

-	for _, target := range targets {
-		foundMap[target] = true
+	expected := []string{"pkg/alpine", "pkg/xen-tools"}
+	sort.Strings(expected)


It is guaranteed that filterPkg returns a sorted array?

@christoph-zededa no, this is why I sort them at lines 63,66

ah, now I see; then it is okay

christoph-zededa · 2024-08-27T08:22:40Z

tools/get-deps/main.go

+				version := matches[versionIndex]
+				// lf-edge version can be sha256 or variable substitution for parse-pkg.sh
+				// in later case it will be empty
+				reSha256 := regexp.MustCompile("^[[:xdigit:]]{1,40}(-amd64|-arm64)?$")


I guess risc V is missing here, isn't it?

rene · 2024-08-27T13:25:35Z

The tool assumed that all lfedge/... docker images are LinuxKit images and there must be pkg/ directory which it not true for e.g. eve-rust container lfedge/eve-rust:1.80.1 Improve version parsing algorithm
@jsfakian I think you also need this PR

I realized that it is easier to check that the pkg/<image> directory exists...

Yeah, it should be easier, FWIW (for other reviewers), the main goal of the tool is to scan Dockerfile files looking for dependencies of all eve-* packages that we built inside eve, so they will be under pkg/. With the introduction of the eve-rust (which AFAIK is now the only one that we build "outside" EVE) the parser wasn't prepared for this new use case and will consider a non-existent pkg/eve-rust, so it must be fixed....

rucoder · 2024-08-27T13:31:00Z

The tool assumed that all lfedge/... docker images are LinuxKit images and there must be pkg/ directory which it not true for e.g. eve-rust container lfedge/eve-rust:1.80.1 Improve version parsing algorithm
@jsfakian I think you also need this PR

I realized that it is easier to check that the pkg/<image> directory exists...

Yeah, it should be easier, FWIW (for other reviewers), the main goal of the tool is to scan Dockerfile files looking for dependencies of all eve-* packages that we built inside eve, so they will be under pkg/. With the introduction of the eve-rust (which AFAIK is now the only one that we build "outside" EVE) the parser wasn't prepared for this new use case and will consider a non-existent pkg/eve-rust, so it must be fixed....

there is another broken case pkg/external-boot-image: pkg/kernel whatever external-boot-image is. I do not think we even use it

If the package is built outside EVE source tree the tool still adds it to dependencies and later Makefile fails to find target. We have lfedge/eve-rust container that is affected but this bug We just check that the package really exists in EVE source tree This commit also fixes the following incorrect dependency pkg/external-boot-image: pkg/kernel Also add some tests Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>

deitch · 2024-08-27T13:35:05Z

which AFAIK is now the only one that we build "outside" EVE

For now 😁

deitch

Much simpler and cleaner.

rene

LGTM

rene · 2024-08-27T13:44:42Z

The tool assumed that all lfedge/... docker images are LinuxKit images and there must be pkg/ directory which it not true for e.g. eve-rust container lfedge/eve-rust:1.80.1 Improve version parsing algorithm
@jsfakian I think you also need this PR

I realized that it is easier to check that the pkg/<image> directory exists...

Yeah, it should be easier, FWIW (for other reviewers), the main goal of the tool is to scan Dockerfile files looking for dependencies of all eve-* packages that we built inside eve, so they will be under pkg/. With the introduction of the eve-rust (which AFAIK is now the only one that we build "outside" EVE) the parser wasn't prepared for this new use case and will consider a non-existent pkg/eve-rust, so it must be fixed....

there is another broken case pkg/external-boot-image: pkg/kernel whatever external-boot-image is. I do not think we even use it

pkg/external-boot-image (kubevirt related) is still built by our Makefile (and has a directory under pkg/), so is valid to check this dependency... pkg/kernel was the old kernel package, but I see that a kind of workaround was introduced here: 6cff366
maybe we could change get-deps to not consider eve-kernel, I think it makes more sense once the kernel is not built under pkg/ anymore.....

rucoder · 2024-08-27T13:49:05Z

The tool assumed that all lfedge/... docker images are LinuxKit images and there must be pkg/ directory which it not true for e.g. eve-rust container lfedge/eve-rust:1.80.1 Improve version parsing algorithm
@jsfakian I think you also need this PR

I realized that it is easier to check that the pkg/<image> directory exists...

Yeah, it should be easier, FWIW (for other reviewers), the main goal of the tool is to scan Dockerfile files looking for dependencies of all eve-* packages that we built inside eve, so they will be under pkg/. With the introduction of the eve-rust (which AFAIK is now the only one that we build "outside" EVE) the parser wasn't prepared for this new use case and will consider a non-existent pkg/eve-rust, so it must be fixed....

there is another broken case pkg/external-boot-image: pkg/kernel whatever external-boot-image is. I do not think we even use it

pkg/external-boot-image (kubevirt related) is still built by our Makefile (and has a directory under pkg/), so is valid to check this dependency... pkg/kernel was the old kernel package, but I see that a kind of workaround was introduced here: 6cff366 maybe we could change get-deps to not consider eve-kernel, I think it makes more sense once the kernel is not built under pkg/ anymore.....

my fix fixes this incorrect dependency

OhmSpectator · 2024-08-28T17:24:16Z

Merging, although there is one test failing. I don't see how it can be related to the PR.

rucoder requested a review from deitch as a code owner August 27, 2024 05:44

rucoder requested review from christoph-zededa, rene and rouming August 27, 2024 05:44

github-actions bot requested review from eriknordmark and jsfakian August 27, 2024 05:45

deitch reviewed Aug 27, 2024

View reviewed changes

christoph-zededa reviewed Aug 27, 2024

View reviewed changes

rucoder force-pushed the rucoder/get-deps branch from 33998a8 to a6c953f Compare August 27, 2024 13:29

github-actions bot requested a review from deitch August 27, 2024 13:29

rucoder requested a review from christoph-zededa August 27, 2024 13:29

rucoder force-pushed the rucoder/get-deps branch from a6c953f to c23efbc Compare August 27, 2024 13:34

deitch approved these changes Aug 27, 2024

View reviewed changes

rene approved these changes Aug 27, 2024

View reviewed changes

OhmSpectator merged commit 0e95a89 into lf-edge:master Aug 28, 2024
38 of 40 checks passed

rucoder added the stable Should be backported to stable release(s) label Aug 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix get-deps tool to handle non LinuxKit containers #4168

Fix get-deps tool to handle non LinuxKit containers #4168

rucoder commented Aug 27, 2024

rucoder commented Aug 27, 2024

deitch Aug 27, 2024

rucoder Aug 27, 2024 •

edited

Loading

deitch Aug 27, 2024

christoph-zededa Aug 27, 2024

rucoder Aug 27, 2024

christoph-zededa Aug 27, 2024

christoph-zededa Aug 27, 2024

rene commented Aug 27, 2024

rucoder commented Aug 27, 2024

deitch commented Aug 27, 2024

deitch left a comment

rene left a comment

rene commented Aug 27, 2024 •

edited

Loading

rucoder commented Aug 27, 2024

OhmSpectator commented Aug 28, 2024

Fix get-deps tool to handle non LinuxKit containers #4168

Fix get-deps tool to handle non LinuxKit containers #4168

Conversation

rucoder commented Aug 27, 2024

rucoder commented Aug 27, 2024

deitch Aug 27, 2024

Choose a reason for hiding this comment

rucoder Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

deitch Aug 27, 2024

Choose a reason for hiding this comment

christoph-zededa Aug 27, 2024

Choose a reason for hiding this comment

rucoder Aug 27, 2024

Choose a reason for hiding this comment

christoph-zededa Aug 27, 2024

Choose a reason for hiding this comment

christoph-zededa Aug 27, 2024

Choose a reason for hiding this comment

rene commented Aug 27, 2024

rucoder commented Aug 27, 2024

deitch commented Aug 27, 2024

deitch left a comment

Choose a reason for hiding this comment

rene left a comment

Choose a reason for hiding this comment

rene commented Aug 27, 2024 • edited Loading

rucoder commented Aug 27, 2024

OhmSpectator commented Aug 28, 2024

rucoder Aug 27, 2024 •

edited

Loading

rene commented Aug 27, 2024 •

edited

Loading