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

Migrate location-related structs to the file package #1751

Merged
merged 13 commits into from May 24, 2023
Merged

Conversation

wagoodman
Copy link
Contributor

@wagoodman wagoodman commented Apr 20, 2023

Partially implements #558 by migrating the following resources from the source package to the file package:

  • Location, Locations, LocationSet
  • Coordinates, CoordinateSet
  • FileResolver, PathResolver, ContentResolver, WritableFileResolver, FileMetadataResolver

In doing so, type aliases have been left behind with deprecation warnings, allowing existing lib users to port over to the new types/functions without introducing a breaking change yet (See syft/source/deprecated.go).

In order to do this, part of the plan in #558 needed to be executed, specifically to split up file type definitions from capabilities (catalogers). This means introducing the same package organization structure that exists today with the package catalogers:

syft/
└── file/
    ├── cataloger/
    │   └── (collections of catalogers, organized in semantically-named packages)
    └── (basic definitions relating to files)

Note: this is a breaking change for any lib consumer that is directly importing the file catalogers (which is not recommended).

I've also updated the existing places in the codebase that were leveraging the newly deprecated source.* types/functions.

File resolver implementations have been moved to the file/resolver package and are all now exported for general use. This is under the same reasoning for splitting up definitions from capabilities, but in this case it's the first step in separating out source.Source definitions from general capabilities of the source object. Since no file resolvers have been exported in the past, this is not a breaking change.

Lastly, a sizable 1.7 MB binary test fixture was removed from the repo 🎉 , but I had to remove an element from the tests that used it to do so.

Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
@github-actions
Copy link

github-actions bot commented Apr 20, 2023

Benchmark Test Results

Benchmark results from the latest changes vs base branch
goos: linux%0Agoarch: amd64%0Apkg: github.com/anchore/syft/test/integration%0Acpu: Intel(R) Xeon(R) Platinum 8171M CPU @ 2.60GHz%0A                                                          │ ./.tmp/benchmark-4c57180.txt │%0A                                                          │            sec/op            │%0AImagePackageCatalogers/alpmdb-cataloger-2                                    14.47m ± 3%25%0AImagePackageCatalogers/apkdb-cataloger-2                                     875.6µ ± 1%25%0AImagePackageCatalogers/binary-cataloger-2                                    248.1µ ± 3%25%0AImagePackageCatalogers/dpkgdb-cataloger-2                                    691.4µ ± 3%25%0AImagePackageCatalogers/dotnet-deps-cataloger-2                               1.471m ± 5%25%0AImagePackageCatalogers/go-module-binary-cataloger-2                          111.0µ ± 2%25%0AImagePackageCatalogers/java-cataloger-2                                      16.42m ± 6%25%0AImagePackageCatalogers/graalvm-native-image-cataloger-2                      108.0µ ± 1%25%0AImagePackageCatalogers/javascript-package-cataloger-2                        491.8µ ± 3%25%0AImagePackageCatalogers/nix-store-cataloger-2                                 337.3µ ± 4%25%0AImagePackageCatalogers/php-composer-installed-cataloger-2                    952.5µ ± 2%25%0AImagePackageCatalogers/portage-cataloger-2                                   572.4µ ± 3%25%0AImagePackageCatalogers/python-package-cataloger-2                            3.944m ± 2%25%0AImagePackageCatalogers/r-package-cataloger-2                                 246.3µ ± 2%25%0AImagePackageCatalogers/rpm-db-cataloger-2                                    643.7µ ± 3%25%0AImagePackageCatalogers/ruby-gemspec-cataloger-2                              1.109m ± 1%25%0AImagePackageCatalogers/sbom-cataloger-2                                      142.5µ ± 2%25%0Ageomean                                                                      737.3µ%0A%0A                                                          │ ./.tmp/benchmark-4c57180.txt │%0A                                                          │             B/op             │%0AImagePackageCatalogers/alpmdb-cataloger-2                                   5.126Mi ± 0%25%0AImagePackageCatalogers/apkdb-cataloger-2                                    205.2Ki ± 0%25%0AImagePackageCatalogers/binary-cataloger-2                                   30.19Ki ± 0%25%0AImagePackageCatalogers/dpkgdb-cataloger-2                                   169.1Ki ± 0%25%0AImagePackageCatalogers/dotnet-deps-cataloger-2                              405.4Ki ± 0%25%0AImagePackageCatalogers/go-module-binary-cataloger-2                         9.906Ki ± 0%25%0AImagePackageCatalogers/java-cataloger-2                                     2.822Mi ± 0%25%0AImagePackageCatalogers/graalvm-native-image-cataloger-2                     8.594Ki ± 0%25%0AImagePackageCatalogers/javascript-package-cataloger-2                       100.9Ki ± 0%25%0AImagePackageCatalogers/nix-store-cataloger-2                                49.13Ki ± 0%25%0AImagePackageCatalogers/php-composer-installed-cataloger-2                   186.8Ki ± 0%25%0AImagePackageCatalogers/portage-cataloger-2                                  120.0Ki ± 0%25%0AImagePackageCatalogers/python-package-cataloger-2                           1.003Mi ± 0%25%0AImagePackageCatalogers/r-package-cataloger-2                                53.29Ki ± 0%25%0AImagePackageCatalogers/rpm-db-cataloger-2                                   180.9Ki ± 0%25%0AImagePackageCatalogers/ruby-gemspec-cataloger-2                             144.0Ki ± 0%25%0AImagePackageCatalogers/sbom-cataloger-2                                     14.20Ki ± 0%25%0Ageomean                                                                     132.7Ki%0A%0A                                                          │ ./.tmp/benchmark-4c57180.txt │%0A                                                          │          allocs/op           │%0AImagePackageCatalogers/alpmdb-cataloger-2                                    87.75k ± 0%25%0AImagePackageCatalogers/apkdb-cataloger-2                                     4.182k ± 0%25%0AImagePackageCatalogers/binary-cataloger-2                                     830.0 ± 0%25%0AImagePackageCatalogers/dpkgdb-cataloger-2                                    3.000k ± 0%25%0AImagePackageCatalogers/dotnet-deps-cataloger-2                               6.338k ± 0%25%0AImagePackageCatalogers/go-module-binary-cataloger-2                           281.0 ± 0%25%0AImagePackageCatalogers/java-cataloger-2                                      39.80k ± 0%25%0AImagePackageCatalogers/graalvm-native-image-cataloger-2                       228.0 ± 0%25%0AImagePackageCatalogers/javascript-package-cataloger-2                        1.404k ± 0%25%0AImagePackageCatalogers/nix-store-cataloger-2                                  895.0 ± 0%25%0AImagePackageCatalogers/php-composer-installed-cataloger-2                    4.079k ± 0%25%0AImagePackageCatalogers/portage-cataloger-2                                   2.269k ± 0%25%0AImagePackageCatalogers/python-package-cataloger-2                            16.44k ± 0%25%0AImagePackageCatalogers/r-package-cataloger-2                                  929.0 ± 0%25%0AImagePackageCatalogers/rpm-db-cataloger-2                                    3.989k ± 0%25%0AImagePackageCatalogers/ruby-gemspec-cataloger-2                              2.447k ± 0%25%0AImagePackageCatalogers/sbom-cataloger-2                                       394.0 ± 0%25%0Ageomean                                                                      2.582k

Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
@wagoodman wagoodman self-assigned this Apr 26, 2023
@wagoodman wagoodman added the deprecated A functionality is being deprecated label Apr 26, 2023
@wagoodman wagoodman changed the title Migrate location-related structs to file package Migrate location-related structs to the file package Apr 26, 2023
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
@wagoodman wagoodman marked this pull request as ready for review May 18, 2023 20:33
@wagoodman wagoodman requested a review from a team May 18, 2023 20:33
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
@wagoodman
Copy link
Contributor Author

Comments from the team during the offline/sync review:

  • do we need to export the resolver implementations? we should try not to
  • do the resolver implementations need to be in file? no, we can keep this in source
  • delete the hardlink test, leave the comment

Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
@spiffcs
Copy link
Contributor

spiffcs commented May 22, 2023

@wagoodman Finished and LGTM after sync review - I'll 🟢 when the comments/changes are pushed from our call

@wagoodman
Copy link
Contributor Author

I made one change relative to the PR comments offline: the resolvers are in syft/internal instead of syft/source/internal. Why? A few reasons:

  1. different than what I described in the review, there are shared concerns for the resolvers. Some resolvers are used directly within the catalogers (Empty and UnindexedDirectory). I could split up these implementations, however, then we'd have something like syft/internal/resolver and syft/source/internal/resolver making importing from both within the source package annoying.
  2. There are shared testing concerns -- the unindexed resolver shares many fixtures with the directory resolver, and for good reason. Copying these fixtures is ok, but again, seemed less ideal the further I got.

Given these points, we still want a spot that keeps resolvers as internal concerns but shared amongst the core API (not the CLI packages). This makes syft/internal the best spot.

I also renamed the new resolver package to fileresolver since it no longer has the benefit of context under the file package.

Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
@wagoodman wagoodman merged commit 07e7690 into main May 24, 2023
9 checks passed
@wagoodman wagoodman deleted the migrate-file-types branch May 24, 2023 21:06
spiffcs added a commit that referenced this pull request Jun 5, 2023
* main: (21 commits)
  chore(deps): bump github.com/sirupsen/logrus from 1.9.2 to 1.9.3 (#1862)
  chore(deps): bump modernc.org/sqlite from 1.22.1 to 1.23.0 (#1863)
  feat: source-version flag (#1859)
  chore(deps): bump github.com/spf13/viper from 1.15.0 to 1.16.0 (#1851)
  accept main.version ldflags even without vcs (#1855)
  feat: add scope to pom properties (#1779)
  chore(deps): bump github.com/stretchr/testify from 1.8.3 to 1.8.4 (#1852)
  chore(deps): bump github.com/docker/docker (#1849)
  Add test to ensure package metadata is represented in the JSON schema (#1841)
  Fix directory resolver to consider CWD and root path input correctly (#1840)
  Migrate location-related structs to the file package (#1751)
  chore(deps): bump github.com/go-git/go-git/v5 from 5.6.1 to 5.7.0 (#1843)
  fix: add panic recovery for license parse (#1839)
  chore: return both failures when failed to retrieve an image with a scheme (#1801)
  Extract go module versions from ldflags for binaries built by go (#1832)
  fix: duplicate packages, support pnpm lockfile v6 (#1778)
  chore(deps): update stereoscope to e14bc4437b2eac481c5b6f101890b22df4f33596 (#1834)
  chore(deps): bump github.com/stretchr/testify from 1.8.2 to 1.8.3 (#1829)
  chore(deps): bump github.com/docker/docker (#1833)
  Keep original FileInfo persisted on file.Metadata structs (#1794)
  ...

Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
GijsCalis pushed a commit to GijsCalis/syft that referenced this pull request Feb 19, 2024
* migrate location structs to file package

Signed-off-by: Alex Goodman <alex.goodman@anchore.com>

* replace source.Location refs with file package call

Signed-off-by: Alex Goodman <alex.goodman@anchore.com>

* fix linting

Signed-off-by: Alex Goodman <alex.goodman@anchore.com>

* remove hardlink test for file based catalogers

Signed-off-by: Alex Goodman <alex.goodman@anchore.com>

* remove hardlink test for all-regular-files testing

Signed-off-by: Alex Goodman <alex.goodman@anchore.com>

* migrate file resolver implementations to separate package

Signed-off-by: Alex Goodman <alex.goodman@anchore.com>

* fix linting

Signed-off-by: Alex Goodman <alex.goodman@anchore.com>

* [wip] migrate resolvers to internal

Signed-off-by: Alex Goodman <alex.goodman@anchore.com>

* migrate resolvers to syft/internal

Signed-off-by: Alex Goodman <alex.goodman@anchore.com>

---------

Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
Signed-off-by: <>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecated A functionality is being deprecated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants