Make the Disco URL configurable in ORT

[m-suchecki]

This PR makes the Disco service URL configurable in JavaBootstrapper class. Refer to the single commits for further information.

Part of #12249.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.21%. Comparing base (c214d6c) to head (2bce5eb).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main   #12257   +/-   ##
=========================================
  Coverage     59.21%   59.21%           
  Complexity     1892     1892           
=========================================
  Files           366      366           
  Lines         13839    13839           
  Branches       1462     1462           
=========================================
  Hits           8195     8195           
  Misses         5115     5115           
  Partials        529      529           

Flag	Coverage Δ
funTest-external-tools	16.13% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[sschuberth]

I'm not sure about this. Do we really need to be able to customize the foojay discoapi URL, or would it be sufficient to use the existing customJdkUrl feature (to point to a download URL on a custom discoapi server)?

[sschuberth]

So far, it (deliberately) was an implementation detail that the JavaBootstrapper uses the foojay discoapi, but now you're exposing that via the signature. The idea was that we could switch from foojay discoapi (which had several issues in the past months) to something else without breaking changes in public API.

[m-suchecki]

I can rename it to something else, like jdkApiUrl or jdkRepoServerUrl, to remove the Foojay exposure.

[sschuberth]

That would change nothing about the fact that the URL needs to point to a Disco-API-compatible server.

[m-suchecki]

Yes, that is true. But we are missing the point that said URL will be a mirror or custom server compatible with currently used API. Be it DiscoAPI or something else in the future. Here, we are providing a possibility to use a similar service, based on the officially used one. After all, it's an optional parameter and don't have to be used.

[sschuberth]

Ah, so if the idea is to evolve JavaBootstrapper into a service-agnostic bootstrapper for JDKs that could also use something else than the Disco API service in the future, I'll all for it. But then then object should probably be turned into a class with a constructor argument that added the service URL (for whatever service it is) to here:

ort/utils/ort/src/main/kotlin/JavaBootstrapper.kt

Line 48 in d77e356

internal val discoService = DiscoService.create(client = OkHttpClientHelper.buildClient())

To override:

ort/clients/foojay/src/main/kotlin/DiscoService.kt

Line 51 in 60316b7

.baseUrl(url ?: DEFAULT_SERVER_URL)

[m-suchecki]

So I made JavaBootstrapper service-agnostic. It takes JdkService interface and calls findPackage(). FoojayJdkService implements it and uses DiscoService to fetch JDKs.

[oheger-bosch]

I am not sure about the current approach with the JdkService abstraction. As it is now, consumers still need to know the underlying implementation since they have to create a JavaBootstrapper instance passing in the service. Also, I am not sure that we are already in the position to design a flexible abstraction as long as we only know a single discovery service implementation.

The original plan was much less ambitious: to only make the URL of the disco service used by JavaBootstrapper configurable. I think, the main problem is that in places whether JavaBootstrapper is used, no configuration mechanism is available that could be used for this purpose.

@sschuberth, do you have an idea how this could be solved without inventing a complete abstraction layer?

[sschuberth]

I'm not sure whether we should add this on the AnalyzerConfiguration level. Not all analyzers / package managers need to be able to bootstrap a JDK, and also the related javaVersion property is defined per-package-manager.

[m-suchecki]

Using AnalyzerConfiguration is the easiest way to set this variable, easily accessible by all package managers and didn't require too many changes. If you insist, it could probably be defined per-package-manager, similar to javaVersion. But it would be a duplicated entry, as I don't think anyone would use different servers for each package manager.

[sschuberth]

Using AnalyzerConfiguration is the easiest way to set this variable,

I believe this is one of the cases where the solution with the least amount of code changes is not the best solution in the sense of maintainability and need-to-know-principle.

[m-suchecki]

I see your point. Then it could be defined per-package-manager, in their config. Would that suffice?

[sschuberth]

That would certainly be better in my view. But personally, I'm still not convinced that this feature is of general use / interest on top of the customJdkUrl feature we already have, at least for GradleInspector.

What about other @oss-review-toolkit/core-devs?

[m-suchecki]

I defined the URL variable (jdkRepositoryUrl) per-package-manager instead of using AnalyzerConfiguration.