Switch to firtool-resolver

[jackkoenig]

This uses https://github.com/jackkoenig/firtool-resolver which I will transfer to be an actual chipsalliance repo once this is merged.

Contributor Checklist

• Did you add Scaladoc to every public function/method?
• Did you add at least one test demonstrating the PR?
• Did you delete any extraneous printlns/debugging code?
• Did you specify the type of improvement?
• Did you add appropriate documentation in docs/src?
• Did you request a desired merge strategy?
• Did you add text to be included in the Release Notes for this change?

Type of Improvement

• API modification

Desired Merge Strategy

• Squash

Release Notes

Whereas in the past, the user was expected to provide the firtool binary
on the PATH, Chisel will now manage the firtool binary automatically.
Chisel users the following 3-step process for determining which binary
to use:

1. If environment variable FIRTOOL_PATH is set, it will check for
   firtool in that directory
2. It will check the classpath for firtool provided by
   chipsalliance/llvm-firtool
3. It will download a default version of chipsalliance/llvm-firtool

Both (2) and (3) will store the firtool binary in a sensible location
depending on the user's operating system. See dirs-dev/directories-jvm
for more information. The user can override this location by setting the
FIRTOOL_CACHE environment variable.

Reviewer Checklist (only modified by reviewer)

• Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
• Did you mark the proper milestone (Bug fix: 3.5.x, 3.6.x, or 5.x depending on impact, API modification or big change: 6.0)?
• Did you review?
• Did you check whether all relevant Contributor checkboxes have been checked?
• Did you do one of the following when ready to merge:
  • Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
  • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

[sequencer]

I wonder how about merging firtool-resolver into the chisel repo?

[jackkoenig]

I wonder how about merging firtool-resolver into the chisel repo?

We could. I sort of like keeping it separate because llvm-firtool (the module for re-publishing firtool binaries as Java artifacts) lives in the same repo as firtool-resolver and thus can use firtool-resolver to test itself.

[sequencer]

my point is, if living in chisel repo, it might be convenient to bump with the ci-nightly branch? It is good for those following the master branch.

[seldridge]

LGTM

It's great to see this finally happening.

[seldridge]

The full path should be unnecessary due to the circt/install-circt action adding it to the path: https://github.com/circt/install-circt/blob/main/action.yml#L73

[seldridge]

Also, consider inlining this step with the previous step as they have the same predicate.

[jackkoenig]

My thinking here was that the above step setting FIRTOOL_PATH will eventually be folded into circt/install-circt, so I factored this in the way such that once that happens, we can just bump the circt/install-circt version and delete the step above and everything will continue to work. Basically writing this code such that the expected next change will be extremely simple, does that make sense?

[seldridge]

I agree that this seems reasonable to do.

That would avoid this type of chained conditional steps. Such chained conditional steps usually seems like an anti-pattern. No real problem with this as-is, though.

[seldridge]

Default may be a bit ambiguous. This is probably fine as-is. Consider either making this empty/not a required option or something more descriptive like "use-firtool-resolver".

[jackkoenig]

Yeah good point. I think I'll go with not-required (thus empty) because that makes more sense that you're not installing a specific input, you're just leaving it up to chisel.

[jackkoenig]

I've marked this for backporting to 3.6. It's technical an API change since firtool on the PATH will now be ignored, but I think it's a worthwhile one for usability. Actually deploying this for 3.6.x and 5.x will require ensuring we have the right versions of firtool published with llvm-firtool

[jackkoenig]

my point is, if living in chisel repo, it might be convenient to bump with the ci-nightly branch? It is good for those following the master branch.

I see your point but I don't think that's going to be an issue. firtool-resolver (and llvm-firtool) are intended to be as independent of the actual APIs they wrap as possible. We may need to update them as firtool evolves packaging-wise (adding more artifacts, etc.) but never with the actual day-to-day APIs. I don't see a problem with keeping it backwards (and forwards) compatible enough to avoid needing to change it with nightly.