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.

Sign up for GitHub

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

Jump to bottom

rstudio: clean up and refactor #357034

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

rstudio: clean up and refactor #357034

wants to merge 5 commits into from
+256 −386

Conversation

TomaSajt
Copy link
Contributor

The first 2 commits should not cause any rebuilds.
The 3rd commit should only cause rstudio-server to rebuild. (Since it now has empty arrays as desktopItems and qtWrapperArgs instead of them being undeclared)

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@TomaSajt TomaSajt marked this pull request as draft November 18, 2024 20:19
@TomaSajt TomaSajt force-pushed the rstudio branch 4 times, most recently from 1064909 to 1da1c18 Compare November 18, 2024 23:29
@ofborg ofborg bot requested a review from ciil November 19, 2024 17:03
@TomaSajt TomaSajt marked this pull request as ready for review November 27, 2024 16:05
@TomaSajt TomaSajt requested a review from drupol November 27, 2024 16:05
b-rodrigues
b-rodrigues approved these changes Dec 4, 2024
View reviewed changes
Copy link
Contributor

@b-rodrigues b-rodrigues left a comment

Choose a reason for hiding this comment

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

LGTM

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 4, 2024
@TomaSajt TomaSajt mentioned this pull request Dec 7, 2024
Draft
13 tasks
@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin and removed 10.rebuild-darwin: 1-10 labels Dec 7, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Dec 7, 2024
@TomaSajt TomaSajt marked this pull request as draft December 8, 2024 11:27
@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin and removed 10.rebuild-darwin: 1-10 labels Dec 8, 2024
@TomaSajt TomaSajt force-pushed the rstudio branch 2 times, most recently from 4f64168 to 0e8c1f9 Compare December 8, 2024 14:52
@TomaSajt TomaSajt changed the title rstudio: clean up and do small refactoring rstudio: clean up and refactor Dec 8, 2024
@TomaSajt TomaSajt marked this pull request as ready for review December 8, 2024 18:04
@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin and removed 10.rebuild-darwin: 1-10 labels Dec 9, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Dec 10, 2024
@b-rodrigues
Copy link
Contributor

pinging @ciil @drupol just to make sure this PR didn't fly under your radar

@TomaSajt
Copy link
Contributor Author

I see that the rstudio server test is failing. I'm assuming it's not supposed to. Will check it out.

@nix-owners nix-owners bot requested a review from jbedo December 18, 2024 22:58
@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin and removed 10.rebuild-darwin: 1-10 labels Dec 18, 2024
@TomaSajt
Copy link
Contributor Author

Turns out rstudioWrapper expected the empty $out/share directory of rstudio-server (which I removed during the refactor) to exist. Now only the non-server version symlinks the share directory.
Fixed and rebased on latest master

@ofborg ofborg bot added 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Dec 19, 2024
@TomaSajt
Copy link
Contributor Author

Rebased on master.

Is everything that worked before this PR still working?
I haven't checked since I can't really program in R and haven't used this program before.

If everything is OK, is there anything stopping us from merging?

@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin and removed 10.rebuild-darwin: 1-10 labels Dec 30, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Dec 31, 2024
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/2175

jbedo
jbedo approved these changes Dec 31, 2024
View reviewed changes
Copy link
Contributor

@jbedo jbedo left a comment

Choose a reason for hiding this comment

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

Tested fully functional, previous issue of the visual editor being broken is resolved. LGTM.

@wegank wegank added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Jan 1, 2025
@TomaSajt
Copy link
Contributor Author

TomaSajt commented Jan 2, 2025

Okay, I'll try to merge it in a day if nobody objects to it.
(though, you can merge sooner if you want to, I just don't like self-merging quickly)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@b-rodrigues b-rodrigues b-rodrigues approved these changes

@jbedo jbedo jbedo approved these changes

@ciil ciil Awaiting requested review from ciil

@drupol drupol Awaiting requested review from drupol

Assignees
No one assigned
Labels
6.topic: R 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@TomaSajt @b-rodrigues @nixos-discourse @drupol @jbedo @wegank