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

code-server: 3.6.0 -> 3.8.0 #110652

Merged
merged 1 commit into from Jan 27, 2021
Merged

code-server: 3.6.0 -> 3.8.0 #110652

merged 1 commit into from Jan 27, 2021

Conversation

dguenther
Copy link
Member

Motivation for this change

Updates code-server to 3.8.0.

3.8.0 removes the need to run VSCode patches and fetch submodules, which simplifies the script a bit.

I added support for Darwin, tested on macOS 10.15.7 -- It required patching the install scripts of a few packages that aren't needed at runtime. I used patch for this rather than sed because it seemed easier to review the changes, but let me know if one or the other is preferred and I can switch it out if necessary.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@dguenther
Copy link
Member Author

@tscholak Thanks for the help getting this package to work on Mac! Tagging you here in case you're still interested.

Also @contrun asked for an update to 3.8.0 on this PR #101628

SuperSandro2000
SuperSandro2000 reviewed Jan 24, 2021
View reviewed changes
pkgs/servers/code-server/default.nix Outdated Show resolved Hide resolved
pkgs/servers/code-server/default.nix Outdated Show resolved Hide resolved
pkgs/servers/code-server/default.nix Show resolved Hide resolved
pkgs/servers/code-server/default.nix Outdated Show resolved Hide resolved
pkgs/servers/code-server/default.nix Outdated Show resolved Hide resolved
pkgs/servers/code-server/default.nix Outdated Show resolved Hide resolved
pkgs/servers/code-server/default.nix Outdated Show resolved Hide resolved
pkgs/servers/code-server/default.nix Outdated Show resolved Hide resolved
pkgs/servers/code-server/default.nix Show resolved Hide resolved
pkgs/servers/code-server/default.nix Outdated Show resolved Hide resolved
@ofborg ofborg bot requested a review from offlinehacker January 24, 2021 02:30
@dguenther dguenther force-pushed the code-server3.8.0 branch 3 times, most recently from 5804fab to 9650850 Compare January 24, 2021 06:29
@tscholak
Copy link

@dguenther yes, definitely interested, thanks for pushing for 3.8.0!

SuperSandro2000
SuperSandro2000 reviewed Jan 25, 2021
View reviewed changes
pkgs/servers/code-server/default.nix Outdated
@@ -1,6 +1,6 @@
{ lib, stdenv, fetchFromGitHub, buildGoModule, makeWrapper, runCommand
, moreutils, jq, git, zip, rsync, pkg-config, yarn, python2
, nodejs-12_x, libsecret, xorg, ripgrep, nettools }:
, nodejs-12_x, libsecret, xorg, ripgrep, darwin}:
Copy link
Member

Choose a reason for hiding this comment

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

Please do not input darwin directly but inherit the frameworks and packages you need.

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely, would you mind sharing an example of how to make it work? I'm pretty new to building packages on Darwin. When I did the following, then ran nix-build ~/repos/nixpkgs -A code-server, it errored with error: anonymous function at /Users/derek/repos/nixpkgs/pkgs/servers/code-server/default.nix:1:1 called without required argument 'AppKit'.

Suggested change
, nodejs-12_x, libsecret, xorg, ripgrep, darwin}:
, nodejs-12_x, libsecret, xorg, ripgrep, AppKit, Cocoa, Security, cctools}:

Copy link
Member

Choose a reason for hiding this comment

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

You need to add the following between the two {} in the call Package call in all-packages:

inherit (darwin.apple_sdk.frameworks AppKit

Copy link
Member Author

Choose a reason for hiding this comment

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

Never mind, I think I got it 🤦 Rebuilding to test now, then will push up an update

Copy link
Member Author

Choose a reason for hiding this comment

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

This has been updated now, thanks for the help!

offlinehacker
offlinehacker reviewed Jan 26, 2021
View reviewed changes
pkgs/servers/code-server/default.nix Outdated
@@ -220,6 +230,6 @@ in stdenv.mkDerivation rec {
homepage = "https://github.com/cdr/code-server";
license = licenses.mit;
maintainers = with maintainers; [ offline ];
platforms = ["x86_64-linux"];
platforms = [ "x86_64-linux" "x86_64-darwin" ];
Copy link
Contributor

Choose a reason for hiding this comment

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

aarch64 is missing in platforms

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 It looks like aarch64 hasn't been in platforms so far. I'll try adding it, but if the build fails, I may take out aarch64 from the yarn package hashes, since I don't currently have a way to work on aarch64 packages.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like it built successfully, excellent

pkgs/servers/code-server/default.nix
@@ -129,6 +130,9 @@ in stdenv.mkDerivation rec {
'';

configurePhase = ''
# run yarn offline by default
echo '--install.offline true' >> .yarnrc
Copy link
Contributor

Choose a reason for hiding this comment

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

can you use yarn config set here?

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I can tell, this flag is only available in .yarnrc, not in the config -- I tried using yarn config set instead, but it seemed to have no effect.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@ofborg ofborg bot requested a review from offlinehacker January 26, 2021 17:55
offlinehacker
offlinehacker approved these changes Jan 27, 2021
View reviewed changes
Copy link
Contributor

@offlinehacker offlinehacker left a comment

Choose a reason for hiding this comment

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

looks good to me

SuperSandro2000
SuperSandro2000 reviewed Jan 27, 2021
View reviewed changes
pkgs/servers/code-server/default.nix Outdated
};

cloudAgent = buildGoModule rec {
pname = "cloud-agent";
version = "0.1.0";
version = "0.2.1";
commit = "5beac91dd5feab9c745d58fda8118a0efaa1ef74";
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this? I seems to be unused.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 Deleted this

pkgs/servers/code-server/default.nix Outdated
Comment on lines 86 to 87
]
;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
]
;
];

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@dguenther
Copy link
Member Author

Squashed the commits down, thanks all for the review!

@ofborg ofborg bot requested a review from offlinehacker January 27, 2021 17:25
@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review which is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).
If you have any questions or problems please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 110652 run on x86_64-darwin 1

1 package built:
  • code-server

@SuperSandro2000 SuperSandro2000 merged commit 313edd0 into NixOS:master Jan 27, 2021
@dguenther dguenther deleted the code-server3.8.0 branch October 27, 2021 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@offlinehacker offlinehacker offlinehacker approved these changes

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@dguenther @tscholak @SuperSandro2000 @mudrii @offlinehacker