feat: add find command #99

rebornplusplus · 2023-10-12T08:44:00Z

This PR adds the find command which can be used to query the chisel releases for available slices.

Usage:
  chisel find [find-OPTIONS] [<query>...]

The find command queries the chisel releases for available slices.

With the --release flag, it queries for slices in a particular branch
of chisel-releases repository[1] or a particular directory. If left
unspecified, it queries with the release info found in /etc/lsb-release.

[1] https://github.com/canonical/chisel-releases

[find command options]
      --release=<branch|dir>      Chisel release branch or directory

Examples

$ chisel find --release ubuntu-20.04 python3 2>/dev/null
Slice                                    Package               Release
libpython3.8-minimal_config              libpython3.8-minimal  ubuntu-20.04
libpython3.8-minimal_libs                libpython3.8-minimal  ubuntu-20.04
libpython3.8-stdlib_all-os               libpython3.8-stdlib   ubuntu-20.04
...
... (trimmed) ...
...
python3.8_core                           python3.8             ubuntu-20.04
python3.8_standard                       python3.8             ubuntu-20.04
python3.8_utils                          python3.8             ubuntu-20.04

$ chisel find libc6_libs 2>/dev/null
Slice       Package  Release
libc6_libs  libc6    ubuntu-22.04

$ chisel find foo bar 2>/dev/null
No matching slices for "foo bar"

Please let me know what you think of the changes. I think I could use some suggestions with naming overall, and particularly the value of maxStrDist constant and logic in matchSlice in /cmd/chisel/cmd_find.go.

Have you signed the CLA?

This commit adds the ``find`` command which can be used to query the chisel releases for available slices. --- Usage: chisel find [find-OPTIONS] [<query>...] The find command queries the chisel releases for available slices. With the --release flag, it queries for slices in a particular branch of chisel-releases repository[1] or a particular directory. If left unspecified, it queries with the release info found in /etc/lsb-release. [1] https://github.com/canonical/chisel-releases [find command options] --release=<branch|dir> Chisel release branch or directory

cmd/chisel/cmd_find.go

This commit changes the logic of matching a query against a slice. Following the new behaviour, a slice name will be listed as a match if the query is a sub-string of the slice name, in pkg_slice format. Or, it will be listed if the query is atmost 1 (One) Levenshtein distance [1] away from the slice name, in pkg_slice format. [1] https://en.wikipedia.org/wiki/Levenshtein_distance

3ab898e introduced new changes in behaviour. Fix the tests accordingly.

woky

Can't find anything to nitpick on. LGTM!

cmd/chisel/cmd_find.go

cmd/chisel/cmd_find_test.go

cjdcordeiro

thanks ;)

benhoyt

Looks good to me, just left a couple of nit comments/suggestions.

cmd/chisel/cmd_find.go

cmd/chisel/cmd_help.go

letFunny

Code is looking very good, just left some minor nitpicks and items than can be addressed in other PRs. Some general comment:

I think we do a pretty good job in the help message by directing the user to the chisel-releases repository and telling them that releases match branches. However, I think that we do not do such a great job with error messages. For example:

$ ./chisel find libc6 --release ubuntu
error: invalid release reference: "ubuntu"

There is no mention of the expected format (e.g. name-number), and even with the correct format the error message can be improved, see:

$ ./chisel find libc6 --release ubuntu-11
error: no information for ubuntu-11 release

We could say above what the available releases are or point the user to the repository so he can see the branch names. Because the error messages were not introduced as part of this PR, I think it is best to create an issue and address it in the future, do you agree?

cmd/chisel/cmd_find.go

rebornplusplus · 2023-12-04T10:27:28Z

We could say above what the available releases are or point the user to the repository so he can see the branch names. Because the error messages were not introduced as part of this PR, I think it is best to create an issue and address it in the future, do you agree?

I agree. I will create an issue for this after this PR is closed.

letFunny

Looking good when re-reviewing it. I left a couple of nitpicks. The only thing that I think could be improved in general lines is how we fuzzy match the string but I seem to recall from months ago that we would treat that as a separate PR.

letFunny · 2024-03-07T15:58:57Z

cmd/chisel/cmd_find.go

+		return err
+	}
+	if slices == nil {
+		fmt.Fprintf(Stdout, "No matching slices for \"%s\"\n", query)


can you use fmt.Printf for simplicity?

I am a bit sceptical about doing that since the existing codes make use of the Stdout variable for testing purposes and there are no occurrences of directly using fmt.Print*:

$ grep -rni "fmt.Print" ./ ./internal/jsonwall/jsonwall.go:28:// fmt.Println(app.Name, "version:", app.Version) ./internal/jsonwall/jsonwall.go:37:// fmt.Println(app.Name, "version:", app.Version)

$ grep -rni "fmt.FPrint.*Stdout" ./ ./cmd/chisel/cmd_version.go:31: fmt.Fprintf(Stdout, "%s\n", cmd.Version) ./cmd/chisel/cmd_help.go:177: fmt.Fprintln(Stdout, longChiselDescription) ./cmd/chisel/cmd_help.go:178: fmt.Fprintln(Stdout) ./cmd/chisel/cmd_help.go:179: fmt.Fprintln(Stdout, chiselUsage) ./cmd/chisel/cmd_help.go:180: fmt.Fprintln(Stdout) ./cmd/chisel/cmd_help.go:181: fmt.Fprintln(Stdout, chiselHelpCategoriesIntro) ./cmd/chisel/cmd_help.go:185: fmt.Fprintln(Stdout) ./cmd/chisel/cmd_help.go:186: fmt.Fprintln(Stdout, chiselHelpAllFooter) ./cmd/chisel/cmd_help.go:191: fmt.Fprintln(Stdout, chiselHelpFooter) ./cmd/chisel/cmd_help.go:197: fmt.Fprintln(Stdout) ./cmd/chisel/cmd_help.go:205: fmt.Fprintf(Stdout, "%*s: %s\n", maxLen+2, categ.Label, strings.Join(categ.Commands, ", ")) ./cmd/chisel/cmd_help.go:230: fmt.Fprintln(Stdout) ./cmd/chisel/cmd_help.go:231: fmt.Fprintf(Stdout, " %s (%s):\n", categ.Label, categ.Description) ./cmd/chisel/cmd_help.go:237: fmt.Fprintf(Stdout, " %*s %s\n", -maxLen, name, cmd.ShortDescription)

Then let's keep it like this if it is the convention. Thanks.

Turns out stdout is used in the tests like we discussed weeks ago. This comment can be resolved.

cmd/chisel/cmd_find.go

letFunny · 2024-06-04T09:12:34Z

Had a high level discussion with @niemeyer about the search functionality. We agreed that for the first iteration we should try to keep things simple but also make the UX feel natural to the user.

To achieve that we are going to use a string distance of 1 to see if two strings match. Additionally, we are going to take each term passed to the CLI (i.e. chisel find <term1> <term2> ...) and try to match those against package name or slice name but the important thing is that all of the terms have to match in order to add that slice to the results.

niemeyer

This is looking reasonable, thanks. Only a few nitpicks.

niemeyer · 2024-06-17T17:50:52Z

cmd/chisel/cmd_find.go

+		return err
+	}
+	if len(slices) == 0 {
+		fmt.Fprintf(Stdout, "No matching slices for \"%s\"\n", strings.Join(cmd.Positional.Query, " "))


s/Stdout/Stderr/ -- It's good practice not to send warnings, errors, and prose in general to Stdout when what is expected is structured output. Another option is to do the exact same output, but present an empty list. What do we do in such cases with snapd?

Snap uses stderr, I will change it here.

niemeyer · 2024-06-17T17:53:42Z

cmd/chisel/cmd_find.go

+	w := tabWriter()
+	fmt.Fprintf(w, "Slice\tPackage\tRelease\n")
+	for _, s := range slices {
+		fmt.Fprintf(w, "%s\t%s\t%s\n", s, s.Package, releaseLabel)


This should use the typical package_slice syntax I think?

Also, why are we presenting Release in every line when this was an input parameter and will be the same on every line?

This is conforming to the output format defined in the spec. Example taken from the doc:

Slice Package Release apache2_bins apache2 ubuntu-22.04 apache2_libs apache2 ubuntu-22.04 ....

Let's discuss it and potentially change it.

niemeyer · 2024-06-17T17:55:04Z

cmd/chisel/cmd_find.go

+func match(slice *setup.Slice, query string) bool {
+	const maxStrDist = 1
+	fuzzyMatch := func(str, query string) bool {
+		return strdist.Distance(str, query, strdist.StandardCost, maxStrDist+1) <= maxStrDist


Looks like this should be a normal function instead of a closure.

This is no longer applicable with the latest changes.

niemeyer · 2024-06-17T18:00:57Z

cmd/chisel/cmd_find.go

+}
+
+// findSlices goes through the release searching for any slices that match
+// the query string. It returns a list of slices that match the query.


// findSlices returns slices from the provided release that contain all of the query strings (AND).

Done in 1186a98.

niemeyer · 2024-06-17T18:02:10Z

cmd/chisel/cmd_find.go

+
+// findSlices goes through the release searching for any slices that match
+// the query string. It returns a list of slices that match the query.
+func findSlices(release *setup.Release, queries []string) (slices []*setup.Slice, err error) {


s/queries/query/ (it's one query with several required strings, not several independent queries, which would hint at an OR relationship)

Done in 1186a98.

niemeyer · 2024-06-17T18:13:16Z

cmd/chisel/cmd_find.go

+	fuzzyMatch := func(str, query string) bool {
+		return strdist.Distance(str, query, strdist.StandardCost, maxStrDist+1) <= maxStrDist
+	}
+	return strings.Contains(slice.String(), query) ||


That doesn't look right. For example, why are we happy to match "ab" against "abcdefghij", but then do a fuzzy match between them and request that the delta is only 1?

AFAIK, snap find looks for substrings as well, if you look for "python" you get:

Name Version Publisher Notes Summary python3-alt 1.0.0 muqtxdir-m - python3 snap packaging python-ai-toolkit 0.1.5.4 mauringo - Collection of common python packages for Data Analysis and AI python0 0.9.1 zygoon classic Ancient version of Python for programming archeologists python36-jamesh 3.6.4 jamesh - Python language interpreter and standard library python2-jamesh 2.7.14 jamesh - Python language interpreter and standard library python38 3.8.0 om26er - Python 3.8 python36-arun 3.6.4 arun.prasad - Python language interpreter and standard library

The difference is that they do not use fuzzy matching at all. Do we want to go that route?

As discussed offline, we will take search for "" for each term in the query. That will allow us to get the substring behavior for free. We will also allow * and ? inside the query.

@niemeyer The only thing I am not sure about is whether we still want to allow a distance of 1 with other characters. In previous versions python3.1x would have matched python3.10 but now that we can write python3.1? I am not sure there is a need for that anymore (in the current PR the distance ONLY takes into account globs).

niemeyer · 2024-06-17T18:16:15Z

cmd/chisel/helpers.go

+			Label:   label,
+			Version: version,
+		})
+		releaseLabel = label + "-" + version


Why do we return a releaseLabel that disagrees with release.Label? At the very least, we should not call this the release label, but maybe we should not return it at all? How's that information used, and what's the best approach considering that?

Agree. I have removed it now that we do not use it for the output so there is no need to think of a clearer name.

niemeyer · 2024-06-17T18:25:51Z

cmd/chisel/helpers.go

+}
+
+// readOrFetchRelease takes a release branch name or a release directory path.
+// It fetches or reads the chisel-release depending on the nature of input and


s/readOrFetch/obtain/?

We always read, maybe local, maybe after fetching, but we don't have to try to convey all these details via the function name.

Then, the documentation needs to follow usual Go conventions, and should describe what it does more clearly. For example:

// obtainRelease returns the Chisel release information matching the provided string, // fetching it if necessary. The provided string should either be a "<name>-<version>" pair, // or the path to a directory containing a previously fetched release.

... or something along those lines.

Done in 99b9496.

niemeyer

Looks good, Alberto. Probably final pass.

niemeyer · 2024-06-28T15:15:29Z

cmd/chisel/cmd_cut.go

 `

 var cutDescs = map[string]string{
-	"release": "Chisel release directory",
+	"release": "Chisel release directory or Ubuntu version",


"Chisel release name or directory (e.g. ubuntu-22.04)"

The directory case is quite special. We don't need to spend much time on it.

Changed in d124e25.

niemeyer · 2024-06-28T15:17:56Z

cmd/chisel/cmd_cut.go

@@ -18,10 +13,13 @@ var shortCutHelp = "Cut a tree with selected slices"
 var longCutHelp = `
 The cut command uses the provided selection of package slices
 to create a new filesystem tree in the root location.
+
+By default it fetches the slices for the latest Ubuntu
+version, unless the --release flag is used.


Is that true? Isn't to actually to fetch slices for the same Ubuntu release as the current host?

You are completely right, changed in d124e25.

niemeyer · 2024-06-28T15:18:21Z

cmd/chisel/cmd_find.go

+Globs (* and ?) are allowed in the query.
+
+By default it fetches the slices for the latest Ubuntu
+version, unless the --release flag is used.


Same as above.

You are completely right, changed in d124e25.

niemeyer · 2024-06-28T15:18:35Z

cmd/chisel/cmd_find.go

+`
+
+var findDescs = map[string]string{
+	"release": "Chisel release directory or Ubuntu version",


Same as above.

Changed in d124e25.

niemeyer · 2024-06-28T15:24:30Z

internal/strdist/strdist.go

+//	?  - Any one character
+//	*  - Any zero or more characters
+//	⁑  - Any zero or more characters
+func StandardGlobCost(ar, br rune) Cost {


And yet, this standard is not what we use in GlobPath. Any reason why we're not just using GlobPath for our match? We won't have slashes in those strings, so it won't affect anything.

I was not comfortable having some logic that was treating "/" differently. For example: python3.1x matches python3.10 where x is any character except when that character is a "/".

How is a slash supposed to be seen at all, in either of these strings? These are package and slice names.

In either case, the "Standard" in the StandardCost function comes from the fact that this is how one would get a classic Levenshtein distance from the books. There's nothing standard about this function. It's not even what we use ourselves in our own glob path matching.

niemeyer

Thanks!

cjdcordeiro requested review from niemeyer and woky October 12, 2023 08:53

cjdcordeiro assigned niemeyer Oct 12, 2023

woky reviewed Oct 12, 2023

View reviewed changes

cmd/chisel/cmd_find.go Outdated Show resolved Hide resolved

rebornplusplus requested review from woky and cjdcordeiro October 13, 2023 09:43

test(cmd/find): fix tests for new matching logic

0858f6a

3ab898e introduced new changes in behaviour. Fix the tests accordingly.

woky approved these changes Oct 13, 2023

View reviewed changes

cjdcordeiro requested changes Oct 13, 2023

View reviewed changes

cmd/chisel/cmd_find.go Outdated Show resolved Hide resolved

cmd/chisel/cmd_find_test.go Outdated Show resolved Hide resolved

rebornplusplus added 2 commits October 16, 2023 13:26

fix(cmd/find): update grammar on find --help

828055c

test(cmd/find): add new test for no matching slices

e84776d

rebornplusplus requested a review from cjdcordeiro October 16, 2023 07:31

cjdcordeiro approved these changes Oct 16, 2023

View reviewed changes

rebornplusplus mentioned this pull request Oct 19, 2023

feat: add info command #101

Merged

1 task

benhoyt approved these changes Oct 19, 2023

View reviewed changes

cmd/chisel/cmd_find.go Outdated Show resolved Hide resolved

cmd/chisel/cmd_find.go Outdated Show resolved Hide resolved

cmd/chisel/cmd_find.go Outdated Show resolved Hide resolved

benhoyt reviewed Oct 19, 2023

View reviewed changes

cmd/chisel/cmd_find.go Outdated Show resolved Hide resolved

benhoyt reviewed Oct 19, 2023

View reviewed changes

cmd/chisel/cmd_find.go Outdated Show resolved Hide resolved

benhoyt reviewed Oct 19, 2023

View reviewed changes

cmd/chisel/cmd_find.go Outdated Show resolved Hide resolved

fix: address feedback from Ben

6d04d2d

cjdcordeiro reviewed Oct 20, 2023

View reviewed changes

cmd/chisel/cmd_help.go Outdated Show resolved Hide resolved

fix(help): move find to the Basic category

03f077a

cjdcordeiro mentioned this pull request Oct 23, 2023

status of database and effort? canonical/chisel-releases#34

Closed

cjdcordeiro added the Priority Look at me first label Oct 26, 2023

letFunny reviewed Nov 29, 2023

View reviewed changes

cmd/chisel/cmd_find.go Show resolved Hide resolved

cmd/chisel/cmd_find.go Outdated Show resolved Hide resolved

cmd/chisel/cmd_find.go Outdated Show resolved Hide resolved

cmd/chisel/cmd_find.go Outdated Show resolved Hide resolved

rebornplusplus added 2 commits December 4, 2023 16:21

refactor: move GetRelease to setup package

67d2f7d

test: adjust tests for previous refactoring

26fa232

rebornplusplus requested a review from letFunny December 4, 2023 10:28

cjdcordeiro added the Priority Look at me first label Jan 16, 2024

cjdcordeiro removed the Priority Look at me first label Feb 8, 2024

cjdcordeiro mentioned this pull request Feb 21, 2024

feat(23.10): add archive for curl and its dependencies canonical/chisel-releases#127

Closed

letFunny approved these changes Mar 7, 2024

View reviewed changes

refactor: reword comments

b8a8450

rebornplusplus requested a review from letFunny March 8, 2024 02:50

cjdcordeiro added the Priority Look at me first label Mar 14, 2024

letFunny added the Blocked Waiting for something external label Jun 4, 2024

letFunny removed the Priority Look at me first label Jun 4, 2024

letFunny added 2 commits June 6, 2024 10:12

address meeting comments

809b008

Merge branch 'main' into ROCKS-809/cmd/find

217bba6

letFunny added Priority Look at me first Simple Nice for a quick look on a minute or two and removed Blocked Waiting for something external Priority Look at me first labels Jun 6, 2024

niemeyer requested changes Jun 17, 2024

View reviewed changes

letFunny added 4 commits June 18, 2024 12:04

change match to add wildcards instead of using substring

1186a98

change output and remove releaseLabel

99b9496

use stderr when no slices match

d8353c8

meeting comments with Cris

a9d8910

letFunny added the Blocked Waiting for something external label Jun 28, 2024

letFunny added 2 commits June 28, 2024 12:19

change algorithm to use distance of 1 and underscores

d0c78e8

update spread tests

fffa3ca

letFunny removed the Blocked Waiting for something external label Jun 28, 2024

niemeyer requested changes Jun 28, 2024

View reviewed changes

letFunny added 2 commits June 28, 2024 17:34

help strings

d124e25

move cost to private funciton

7dbe374

niemeyer approved these changes Jun 28, 2024

View reviewed changes

niemeyer merged commit 017b616 into canonical:main Jun 28, 2024
14 checks passed

feat: add find command #99

feat: add find command #99

Conversation

rebornplusplus commented Oct 12, 2023 • edited Loading

Examples

woky left a comment

Choose a reason for hiding this comment

cjdcordeiro left a comment

Choose a reason for hiding this comment

benhoyt left a comment

Choose a reason for hiding this comment

letFunny left a comment

Choose a reason for hiding this comment

rebornplusplus commented Dec 4, 2023

letFunny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

letFunny commented Jun 4, 2024

niemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemeyer left a comment

Choose a reason for hiding this comment

rebornplusplus commented Oct 12, 2023 •

edited

Loading