cmdstan_version_compare() returns -1 when the first argument is missing, NA, or an empty string because path discovery uses "" when it doesn't find native or WSL installs. That works well for cmdstan_default_path(). But -1 is also used for indicating that a version is older and it's not possible after the fact to tell the two uses of -1 apart.
I think cmdstan_version_compare() should just compare versions. cmdstan_default_path() can then just return early when it needs to, e.g.:
if (!nzchar(latest_cmdstan)) return(file.path(wsl_installs_path, latest_wsl_cmdstan))
if (!nzchar(latest_wsl_cmdstan)) return(file.path(installs_path, latest_cmdstan))
if (cmdstan_version_compare(latest_wsl_cmdstan, latest_cmdstan) >= 0) ...
We also need to be careful about zzz.R since it runs during .onAttach(). Right now the comparison isn't wrapped in try() the way the version lookups are, so it should probably be moved inside try() or handled some other way.
When running this by claude it pointed out that there's a tempting fix that should be avoided. In it's own words:
Making model_compile_info() return NULL instead of ".." looks cheaper and is worse. NULL hits the sentinel, so $sample()'s gate at R/model.R:1391 silently takes the "older than 2.36.0" branch instead of erroring. That trades a loud failure in the wrong place for a quiet wrong answer.
Refusing bad versions at the source belongs with the executable adoption work, which should decline to construct a model from a binary that cannot report a version (related: #1246). This issue is about the comparison, which should not answer a question it was not asked.
cmdstan_version_compare()returns -1 when the first argument is missing, NA, or an empty string because path discovery uses""when it doesn't find native or WSL installs. That works well forcmdstan_default_path(). But -1 is also used for indicating that a version is older and it's not possible after the fact to tell the two uses of -1 apart.I think
cmdstan_version_compare()should just compare versions.cmdstan_default_path()can then just return early when it needs to, e.g.:We also need to be careful about zzz.R since it runs during
.onAttach(). Right now the comparison isn't wrapped in try() the way the version lookups are, so it should probably be moved insidetry()or handled some other way.When running this by claude it pointed out that there's a tempting fix that should be avoided. In it's own words: