Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle unsupported R versions; prefer current "current" version of R #2652

Merged
merged 9 commits into from
Apr 5, 2024

Conversation

jennybc
Copy link
Member

@jennybc jennybc commented Apr 4, 2024

Addresses #2440, addresses #2535. Also related to #2316 and #2659.

QA notes

Main goal: stop offering to launch unsupported versions of R

  1. Install an unsupported version of R, presumably alongside at least 1 supported version. I would personally use rig for this. Unsupported is anything prior to 4.2. rig add 4.1.3 would do the trick. rig available can be handy.
  2. Launch Positron.
  3. The unsupported version of R should not appear in the interpreter dropdown.
  4. The Positron R Extension output channel should show us discovering the unsupported version of R, but ultimately filtering it out because the version < 4.2.

Secondary goal: Keep launching the current version of R even if that changes

  1. Open Positron in an R-flavored workspace. Make sure the automatically launched R version is indeed the current default on your system. Quit Positron. The point of this is to guarantee that the stored metadata includes current: true.
  2. Change your default version of R. For example, use rig add devel then rig default devel. rig list is useful for checking things.
  3. Relaunch Positron. Presumably it opens the same workspace as in step 1. It should, however, fire up R devel this time, because that is the current "current" version of R.

Secondary goal: Still possible to use non-current version of R

Continuing from above ...

  1. Use the interpreter dropdown to select R release (4.3.3 today), which is not currently the default. This is how you say "no, I really do want 4.3.3 in this workspace, even if it is not the current version of R".
  2. Quit Positron. The point of this is to guarantee that the stored metadata includes current: false.
  3. Relaunch Positron. Presumably it opens in the same workspace as before. It should fire up R release, because the stored metadata had current: false so the bin path should be taken literally.

@@ -6,3 +6,6 @@ import * as path from 'path';

// The extension root directory.
export const EXTENSION_ROOT_DIR = path.join(__dirname, '..');

// The minimum supported version of R
export const MINIMUM_R_VERSION = '4.2.0';
Copy link
Member Author

@jennybc jennybc Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want this to be some Magic Number inlined in provider.ts, r-installation.ts, and the like. Is there an even more central place to put this? I could imagine it being useful outside of the positron-r code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could put it in the package.json file, which is readily readable in ts files via require and could also be consumed easily from outside them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I did that. It looks like we already have positron for custom data, so I added there. Are you OK with the mechanics of how I did this?

Comment on lines +63 to +67
// TODO: possible location to tell the user why certain R installations are being omitted from
// the interpreter drop-down and, in some cases, offer to help fix the situation:
// * version < minimum R version supported by positron-r
// * (macOS only) version is not orthogonal and is not the current version of R
// * invalid R installation
Copy link
Member Author

@jennybc jennybc Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This enhanced experience is covered by #1397. For now, the UX, such as it is, is logging rejected R installations in the output channel.

extensions/positron-r/src/runtime-manager.ts Outdated Show resolved Hide resolved
Comment on lines +62 to +73
let inst: RInstallation;
if (curBin && metadataExtra.current) {
// If the metadata says that it represents the "current" version of R, interpret that to
// mean the current "current" version of R, at this very moment, not whatever it was
// when this metadata was stored.
// The motivation for this mindset is immediate launch of an affiliated runtime.
// More thoughts in this issue:
// https://github.com/posit-dev/positron/issues/2659
inst = new RInstallation(curBin, true);
} else {
inst = new RInstallation(metadataExtra.binpath, curBin === metadataExtra.binpath);
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementing this new idea that most users probably want the current version of R, if they were running the current version R last time, even if current has incremented from, e.g., 4.3 to 4.4 in the meantime.

I opened #2659 for continued discussion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now know this was written down in #2535.

@jennybc jennybc marked this pull request as ready for review April 4, 2024 23:20
@jennybc jennybc requested a review from jmcphers April 4, 2024 23:26
@@ -6,3 +6,6 @@ import * as path from 'path';

// The extension root directory.
export const EXTENSION_ROOT_DIR = path.join(__dirname, '..');

// The minimum supported version of R
export const MINIMUM_R_VERSION = '4.2.0';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could put it in the package.json file, which is readily readable in ts files via require and could also be consumed easily from outside them.

@jmcphers
Copy link
Collaborator

jmcphers commented Apr 5, 2024

Feels like this addresses #2535 too -- do you agree?

@jennybc
Copy link
Member Author

jennybc commented Apr 5, 2024

Feels like this addresses #2535 too -- do you agree?

YES. I remembered this, but apparently filed it away as a conversation and did not realize there was an actual issue. Thanks for connecting that.

@jennybc jennybc changed the title Flag, log, and omit unsupported R versions Handle unsupported R versions; prefer current "current" version of R Apr 5, 2024
@jennybc jennybc requested a review from jmcphers April 5, 2024 20:22
Copy link
Collaborator

@jmcphers jmcphers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jennybc jennybc merged commit 8785f43 into main Apr 5, 2024
1 check passed
@jennybc jennybc deleted the feature/flag-unsupported-r-versions branch April 5, 2024 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants