dnfjson: skip TestDepsolver() if no osbuild-depsolve-dnf is installed

[mvo5]

Do not fail if osbuild-depsolve-dnf is missing.

It also fixes dnfjson.go so that the comment (and I think intention) matches the code :)

[bcl]

Looks good to me. The tests run with -force-dnf so we won't accidentally pass if it's missing :)

[achilleas-k]

Maybe we should remove the python3 -c "import dnf" check now. The osbuild-depsolve-dnf package should depend on libdnf so all we need to do is check if that exists. The shelling out to python thing is a bit silly.

It also fixes dnfjson.go so that the comment (and I think intention) matches the code :)

aaah yeah the comment matches my initial idea and then I thought it might be better to set the the default so that, when (if) it fails, you get an error that shows the primary default location. I'm not sure which is better.

[mvo5]

Maybe we should remove the python3 -c "import dnf" check now. The osbuild-depsolve-dnf package should depend on libdnf so all we need to do is check if that exists. The shelling out to python thing is a bit silly.

Thanks, I think that makes sense, I updated the PR.

It also fixes dnfjson.go so that the comment (and I think intention) matches the code :)

aaah yeah the comment matches my initial idea and then I thought it might be better to set the the default so that, when (if) it fails, you get an error that shows the primary default location. I'm not sure which is better.

I have no strong opinion either way, I pushed a small commit that makes New{,Base}Solver() return an error when no osbuild-depsolve-dnf is found with a descriptive error message. But it has drawbacks in the test (as more are skipped now) so I can just drop this commit again if desired (it feels like the cleanest option though).

[achilleas-k]

I pushed a small commit that makes New{,Base}Solver() return an error when no osbuild-depsolve-dnf is found with a descriptive error message.

We don't want this. It makes it impossible (or harder) to set up the solver externally with a depsolver in a non-standard location. For example, this should be possible:

solver := NewBaseSolver(cacheDir)
solver.SetDNFJSONPath("/some/odd/path/to/solver")

without having osbuild-depsolve-dnf installed, either for testing or for non-standard setups.

So there's a question now about what findDepsolveDnf() should do if the depsolver isn't found:

1. Return the default and let callers change it or fail when run() is called (which will show the primary default location in the error message). This is what happens now in main but admittedly it's not the best since it relies on non-local behaviour for error messaging.
2. Return an empty string and either:
   2.1 Let run() fail (if the path isn't changed) with an empty path in the error message.
   2.2 Make run() check if it's empty and print an appropriate message that it's unset.
3. Return an error but catch it in the caller and set the path to an empty slice (and then do 2.2).

EDIT: Oh...

images/pkg/dnfjson/dnfjson.go

Lines 639 to 641 in 721b201

	if len(dnfJsonCmd) == 0 {
	return nil, fmt.Errorf("osbuild-depsolve-dnf command undefined")
	}

Just set it to an empty slice then.

[achilleas-k]

See #321 (comment)

[mvo5]

I pushed a small commit that makes New{,Base}Solver() return an error when no osbuild-depsolve-dnf is found with a descriptive error message.

We don't want this. It makes it impossible (or harder) to set up the solver externally with a depsolver in a non-standard location. For example, this should be possible:

solver := NewBaseSolver(cacheDir)
solver.SetDNFJSONPath("/some/odd/path/to/solver")

[..]

Thank you, that is an interesting use-case and I have no considered it. I tweaked the PR slightly to run the findDepsolverDnf later. This seems to be the most minimal change and I pushed it. But below another idea if you prefer to detect the missing dnfjson early :)

If we don't want this and make any detection of missing dnfjson part of the constructor I wonder if it would make sense to change the signature of NewBaseSolver() to make sure that the solver path is passed in when constructing? Something like:

diff --git a/pkg/dnfjson/dnfjson.go b/pkg/dnfjson/dnfjson.go
index 9505e09b3..d11ce164a 100644
--- a/pkg/dnfjson/dnfjson.go
+++ b/pkg/dnfjson/dnfjson.go
@@ -61,13 +61,24 @@ func findDepsolveDnf() (string, error) {
        return "", fmt.Errorf("cannot find required osbuild-depsolve-dnf in any of %q", depSolveDnfPaths)
 }
 
+type SolverOptions struct {
+       DNFJsonPath string
+}
+
 // Create a new unconfigured BaseSolver (without platform information). It can
 // be used to create configured Solver instances with the NewWithConfig()
 // method.
-func NewBaseSolver(cacheDir string) (*BaseSolver, error) {
-       depsolveDnf, err := findDepsolveDnf()
-       if err != nil {
-               return nil, err
+func NewBaseSolver(cacheDir string, opts *SolverOptions) (*BaseSolver, error) {
+       if opts == nil {
+               opts = &SolverOptions{}
+       }
+       depsolveDnf := opts.DNFJsonPath
+       if depsolveDnf == "" {
+               foundSolver, err := findDepsolveDnf()
+               if err != nil {
+                       return nil, err
+               }
+               depsolveDnf = foundSolver
        }
        return &BaseSolver{
                cache:       newRPMCache(cacheDir, 1024*1024*1024), // 1 GiB
@@ -135,7 +146,7 @@ type Solver struct {
 
 // Create a new Solver with the given configuration. Initialising a Solver also loads system subscription information.
 func NewSolver(modulePlatformID, releaseVer, arch, distro, cacheDir string) (*Solver, error) {
-       s, err := NewBaseSolver(cacheDir)
+       s, err := NewBaseSolver(cacheDir, nil)
        if err != nil {
                return nil, err
        }

or is an API break like this too annoying?

[achilleas-k]

or is an API break like this too annoying?

Not that annoying. We'll have to change all the calls in composer and bib when we pull in the change but it's not that much work. There's an interesting use case that gets a bit dirty when you want to init the Solver without a solver though (either because you don't intend to use it but need an instance for a test or because you might want to "discover" the solver path later), where the caller would have to do something like:

NewBaseSolver(cacheDir, &SolverOptions{DNFJsonPath: "NOTEMPTY"})

and then change it.

Would we also add the options to the NewSolver() function?

[achilleas-k]

Current state LGTM