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.

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

don't SCA-generate so: or pc: provides for libs not directly in lib dirs #1372

Merged
merged 5 commits into from
Jul 15, 2024

Conversation

imjasonh
Copy link
Member

@imjasonh imjasonh commented Jul 11, 2024

#1370

This changes our logic for identifying provided .sos to only look for them directly in lib dirs, and not nested under those dirs. When one is found in a nested dir, it's moved to vendored instead, where they end up as comments in .PKGINFO, and don't affect the dep graph.

This also updates the tests added in #1369 to actually get generated (while they were in e2e_test.go you needed go generate -tags=e2e), and moves the tests that cover them into non-build-tagged sca_test.go.

This change updates make test-e2e to go generate ./... to run builds and fetch generated requirements, then runs e2e tests on them.

@xnox
Copy link
Contributor

xnox commented Jul 11, 2024

The general case is fine.

About these things in Wolfi APK.

Is this python extensions or their shared library depends? Did we compile them or are they not built from source?

Because most of the time, to be ABI compatible I would expect these to all come from wolfi, and be Depends: so:libquadmath.so.0=0 and some such. Rather than vendored provides.

@xnox
Copy link
Contributor

xnox commented Jul 11, 2024

Btw, in general so: provides should only be for public libraries, shipped in oublically resolvable locations.

Copy link
Contributor

@xnox xnox left a comment

Choose a reason for hiding this comment

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

This fix misses the general point - subdirectories of libDirs are not in ld.so search path and thus should not generate public so: provides.

pkg/sca/sca.go Outdated Show resolved Hide resolved
@imjasonh imjasonh changed the title don't SCA-generate so: provides for libs in site-packages @imjasonh don't SCA-generate so: provides for libs not directly in lib dirs Jul 12, 2024
@imjasonh imjasonh changed the title @imjasonh don't SCA-generate so: provides for libs not directly in lib dirs don't SCA-generate so: provides for libs not directly in lib dirs Jul 12, 2024
Copy link
Contributor

@xnox xnox left a comment

Choose a reason for hiding this comment

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

Looking more into this, it is also true for pcDirs and pkg-config too.

Can you please also use isInDir there, and thus complete remove allowedPrefix() function altogether?

Such that we don't have to deal with this bug again for pc-config files in the future, or ever introduce any new (naive) usage of allowedPrefix().

Signed-off-by: Jason Hall <jason@chainguard.dev>
Signed-off-by: Jason Hall <jason@chainguard.dev>
@imjasonh imjasonh changed the title don't SCA-generate so: provides for libs not directly in lib dirs don't SCA-generate so: or pc: provides for libs not directly in lib dirs Jul 12, 2024
@imjasonh
Copy link
Member Author

Looking more into this, it is also true for pcDirs and pkg-config too.

Can you please also use isInDir there, and thus complete remove allowedPrefix() function altogether?

Such that we don't have to deal with this bug again for pc-config files in the future, or ever introduce any new (naive) usage of allowedPrefix().

Excellent call, done.

I didn't add a test for the change to pkg-config finding, but I did check our build logs for the last 10 days of world builds and there were no matches for textPayload=~"found pkg-config .* for usr/lib/.*/.*/.*" which would indicate that any pc: provides were generated for more deeply nested findings.

Signed-off-by: Jason Hall <jason@chainguard.dev>
Signed-off-by: Jason Hall <jason@chainguard.dev>
@imjasonh imjasonh marked this pull request as ready for review July 12, 2024 14:00
@imjasonh
Copy link
Member Author

For posterity, changing the isInDir back to allowedPrefix results in this test failure and diff:

    --- FAIL: TestAnalyze/py3-seaborn-0.13.2-r0.apk (21.96s)
        e2e_test.go:208: Analyze(): (-want, +got):
              config.Dependencies{
                Runtime: {"so:ld-linux-x86-64.so.2", "so:libXau-154567c4.so.6.0.0", "so:libbrotlicommon-3ecfe81c.so.1", "so:libbrotlidec-ba690955.so.1", ...},
                Provides: []string{
                        ... // 3 identical elements
                        "cmd:pyftsubset=0.13.2-r0",
                        "cmd:ttx=0.13.2-r0",
            +           "so:libXau-154567c4.so.6.0.0=6.0.0",
            +           "so:libbrotlicommon-3ecfe81c.so.1=1",
            +           "so:libbrotlidec-ba690955.so.1=1",
            +           "so:libfreetype-f154df84.so.6.20.1=6.20.1",
            +           "so:libgfortran-040039e1.so.5.0.0=5.0.0",
            +           "so:libharfbuzz-2093a78b.so.0.60830.0=0.60830.0",
            +           "so:libjpeg-e44fd0cd.so.62.4.0=62.4.0",
            +           "so:liblcms2-e69eef39.so.2.0.16=2.0.16",
            +           "so:liblzma-13fa198c.so.5.4.5=5.4.5",
            +           "so:libopenblas64_p-r0-0cf96a72.3.23.dev.so=0",
            +           "so:libopenjp2-eca49203.so.2.5.0=2.5.0",
            +           "so:libpng16-78d422d5.so.16.40.0=16.40.0",
            +           "so:libquadmath-96973f99.so.0.0.0=0.0.0",
            +           "so:libsharpyuv-20f78091.so.0.0.1=0.0.1",
            +           "so:libtiff-91af027d.so.6.0.2=6.0.2",
            +           "so:libwebp-850e2bec.so.7.1.8=7.1.8",
            +           "so:libwebpdemux-df9b36c7.so.2.0.14=2.0.14",
            +           "so:libwebpmux-9fe05867.so.3.0.13=3.0.13",
            +           "so:libxcb-f0538cc0.so.1.1.0=1.1.0",
                },
                Replaces:         nil,
                ProviderPriority: "",
                ReplacesPriority: "",
            -   Vendored: []string{
            -           "so:libXau-154567c4.so.6.0.0=6.0.0", "so:libbrotlicommon-3ecfe81c.so.1=1",
            -           "so:libbrotlidec-ba690955.so.1=1", "so:libfreetype-f154df84.so.6.20.1=6.20.1",
            -           "so:libgfortran-040039e1.so.5.0.0=5.0.0",
            -           "so:libharfbuzz-2093a78b.so.0.60830.0=0.60830.0",
            -           "so:libjpeg-e44fd0cd.so.62.4.0=62.4.0",
            -           "so:liblcms2-e69eef39.so.2.0.16=2.0.16", "so:liblzma-13fa198c.so.5.4.5=5.4.5",
            -           "so:libopenblas64_p-r0-0cf96a72.3.23.dev.so=0",
            -           "so:libopenjp2-eca49203.so.2.5.0=2.5.0",
            -           "so:libpng16-78d422d5.so.16.40.0=16.40.0",
            -           "so:libquadmath-96973f99.so.0.0.0=0.0.0",
            -           "so:libsharpyuv-20f78091.so.0.0.1=0.0.1", "so:libtiff-91af027d.so.6.0.2=6.0.2",
            -           "so:libwebp-850e2bec.so.7.1.8=7.1.8", ...,
            -   },
            +   Vendored: nil,
              }

    --- FAIL: TestAnalyze/systemd-256.2-r1.apk (10.66s)
        e2e_test.go:208: Analyze(): (-want, +got):
              config.Dependencies{
                Runtime: {"so:ld-linux-x86-64.so.2", "so:libblkid.so.1", "so:libc.so.6", "so:libcap.so.2", ...},
                Provides: []string{
                        ... // 50 identical elements
                        "so:libnss_resolve.so.2=2",
                        "so:libnss_systemd.so.2=2",
            +           "so:libsystemd-core-256.so=0",
            +           "so:libsystemd-shared-256.so=0",
                        "so:libudev.so.1=1",
                },
                Replaces:         nil,
                ProviderPriority: "",
                ReplacesPriority: "",
            -   Vendored:         []string{"so:libsystemd-core-256.so=0", "so:libsystemd-shared-256.so=0"},
            +   Vendored:         nil,
              }

Signed-off-by: Jason Hall <jason@chainguard.dev>
@xnox
Copy link
Contributor

xnox commented Jul 12, 2024

With this PR, i can drop no-provides from gcc-12, and it generates correct runtime dependencies and provides, resulting in co-installable gcc-12 and gcc packages; allowing to use both latest and older toolchain.

@xnox xnox merged commit 620c535 into chainguard-dev:main Jul 15, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants