rust-analyzer: 2020-08-24 -> 2020-09-14 and some clean-up #97444

oxalica · 2020-09-08T12:39:35Z

Motivation for this change

Bump version and do some clean-up.

unstable-2020-09-07 requires rustc 1.46.0 to compile but we still have 1.45.2 in master. Patches are introduced for a temporary fix.
Feature jemalloc is removed now, and a new features mimalloc is introduced.
Remove fake rustup in check phase, since we currently test only sub-crate rust-analyzer, which doesn't need it anymore.
Install check works now. Enable it by default.

Things done

danieldk · 2020-09-08T12:54:58Z

This update touches node packages, since I literally know nothing about node, I'll leave up the reviewing to someone else.

midchildan · 2020-09-14T13:08:32Z

pkgs/development/tools/rust/rust-analyzer/generic.nix

@@ -17,26 +18,21 @@ rustPlatform.buildRustPackage {

  buildAndTestSubdir = "crates/rust-analyzer";

-  cargoBuildFlags = lib.optional useJemalloc "--features=jemalloc";
+  cargoBuildFlags = lib.optional useMimalloc "--features=mimalloc";


Note: Upstream PR in rust-lang/rust-analyzer#5354

midchildan · 2020-09-14T13:16:39Z

pkgs/development/tools/rust/rust-analyzer/generic.nix


-  nativeBuildInputs = lib.optionals doCheck [ rustPlatform.rustcSrc ];
+  nativeBuildInputs = lib.optional useMimalloc cmake;


Note: Needed for mimalloc_rust to build its bundled version of the mimalloc library. https://github.com/purpleprotocol/mimalloc_rust#requirements

midchildan · 2020-09-14T13:25:46Z

pkgs/development/tools/rust/rust-analyzer/generic.nix

 # Version specific args
-, rev, version, sha256, cargoSha256 }:
+{ rev, version, sha256, cargoSha256, ... }@args:


Is it necessary to split the arguments? Looking at the output of rg -B3 '@args', it doesn't look like a common pattern in nixpkgs.

midchildan · 2020-09-14T13:31:40Z

pkgs/development/tools/rust/rust-analyzer/default.nix

+    patches = [
+      # FIXME: Temporary fix for our rust 1.45.0 since rust-analyzer requires 1.46.0
+      ./no-loop-in-const-fn.patch
+      ./no-option-zip.patch
+    ];


I think this would better belong in generic.nix, along with other build instructions.

🤔 But it's temporary.

Well, I'm not quite sure what should be put in generic file. You mean all about "how to build", yeah?

I'd expect most patches are supposed to be temporary workarounds, so it wouldn't be a problem as long as there's a comment explaining why they're necessary. If you want to make absolutely sure that you remove the patch when it's no longer needed, I guess you could do something like this:

patches = with lib; if !versionAtLeast (getVersion rustPlatform.rust.rustc) "1.46.0" then [ # FIXME: Temporary fix for our rust 1.45.0 since rust-analyzer requires 1.46.0 ./no-loop-in-const-fn.patch ./no-option-zip.patch ] else warn "Some patches for ${pname} needs to be removed." [];

(edited, it was missing a !)

This would exclude the patches from the build and warn you about it once rustc reaches 1.46.0.

You mean all about "how to build", yeah?

Yes.

If you want to make absolutely sure that you remove the patch when it's no longer needed, I guess you could do something like this

These patches just do the same thing in a more compatible way and don't change any semantics. I think it's not quite necessary to do that check.

That makes sense.

midchildan · 2020-09-15T13:23:21Z

pkgs/development/tools/rust/rust-analyzer/generic.nix

 # Version specific args
-, rev, version, sha256, cargoSha256 }:
+{ rev, version, sha256, cargoSha256 }:
+
+{ lib, stdenv, fetchFromGitHub, rustPlatform, darwin, cmake
+, useMimalloc ? false
+, doCheck ? true
+}:


Could you also make this a simple function instead of a nested one?

Is it necessary? Some are just extracted version related parameters, while others are passed through callPackages which can be overrided. It's not expected to directly configure rev, version and hashes like other packages in nixpkgs.

It's only a matter of style but I found the original version easier to read than the nested version, although they basically do the same thing.

original:

rust-analyzer-unwrapped = callPackage ./generic.nix rec { rev = ...; ... };

nested:

rust-analyzer-unwrapped = callPackage (import ./generic.nix rec { rev = ...; ... }) {};

midchildan

LGTM. The remaining point is a minor style issue, so I don't see much point holding this up.

midchildan · 2020-09-15T19:32:20Z

@oxalica This still needs to go through committer review, but you might want to squash the commits in the meantime.

oxalica · 2020-09-16T17:39:34Z

The global node-packages (pkgs/development/node-packages/node-packages.nix) is updated too frequently.
I'm thinking about using a standalone generated node-packages.nix for the package.

What's the best practice about these npm packages as only build-dependency?

@jonringer

oxalica · 2020-09-17T19:07:24Z

I refactored the node packages part to use generated standalone node-packages.nix to avoid touching global nodePackages.

@midchildan Have a look? What do you think about it?

midchildan · 2020-09-18T16:37:22Z

Using node2nix outside of nodePackages is generally frowned upon [1,2] even if it's only used as a build dependency [3,4]. The guideline for node.js packaging [5], however, states that "the package set should only provide end user software package," and this has been pointed out in another issue [6], but I think it's best to stick with the currently accepted practice for now because no decision has been reached yet.

oxalica · 2020-09-18T17:03:00Z

@midchildan I'm just tired of generating and generating again over ALL NPM packages used by nixpkgs every time having merge conflict (it's updated quite often). We have package-lock.json but node2nix ignores them and fetches all packages every time, which takes about an hour for me and (accidentally) cause ~100 package rebuild (which may be not needed or even lead to build failure)

(Also node2nix is poor designed and the fetching is synchronous. It takes years in a proxy environment with high latency but large bandwidth.)

Note that just when I typing this comment, there's another update on pkgs/development/node-packages/node-packages.nix.
MERGE CONFLICT AGAIN 😞

midchildan · 2020-09-18T17:44:21Z

The current situation isn't ideal, but I believe this discussion would be best done in a separate issue because this affects how node packaging should be done in general, and isn't limited to this package. #82662 seems like a good place to start.

For now, I'd recommend ignoring merge conflicts until you receive committer approval. You can perform a rebase afterwards.

jonringer

LGTM

failures are broken on target branch

https://github.com/NixOS/nixpkgs/pull/97444
2 packages failed to build:
dat vimPlugins.coc-imselect

17 packages built:
bitwarden-cli castnow epgstation joplin lumo mirakurun netlify-cli redoc-cli rust-analyzer rust-analyzer-unwrapped thelounge vimPlugins.coc-eslint vimPlugins.coc-git vimPlugins.coc-metals vimPlugins.coc-prettier vimPlugins.coc-stylelint vimPlugins.coc-vetu

oxalica requested a review from jonringer as a code owner September 8, 2020 12:39

ofborg bot added 11.by: package-maintainer 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 labels Sep 8, 2020

oxalica mentioned this pull request Sep 14, 2020

rust-analyzer: requires cargo in path to work properly #97926

Closed

oxalica force-pushed the rust-analyzer branch from 9b755ba to 96f7c95 Compare September 14, 2020 11:51

ofborg bot requested a review from midchildan September 14, 2020 12:32

midchildan reviewed Sep 14, 2020

View reviewed changes

midchildan suggested changes Sep 14, 2020

View reviewed changes

ofborg bot requested a review from midchildan September 15, 2020 11:42

oxalica changed the title ~~rust-analyzer: 2020-08-24 -> 2020-09-07 and some clean-up~~ rust-analyzer: 2020-08-24 -> 2020-09-14 and some clean-up Sep 15, 2020

midchildan suggested changes Sep 15, 2020

View reviewed changes

midchildan approved these changes Sep 15, 2020

View reviewed changes

oxalica force-pushed the rust-analyzer branch from 50793c7 to cc5262f Compare September 17, 2020 19:02

ofborg bot requested a review from midchildan September 17, 2020 20:04

rust-analyzer: 2020-08-24 -> 2020-09-21

e6f871c

oxalica force-pushed the rust-analyzer branch from cc5262f to e6f871c Compare September 21, 2020 13:07

midchildan approved these changes Sep 21, 2020

View reviewed changes

ofborg bot requested a review from midchildan September 21, 2020 13:27

rust-analyzer: fix build

ee7c2fb

midchildan approved these changes Sep 21, 2020

View reviewed changes

jonringer approved these changes Sep 21, 2020

View reviewed changes

jonringer merged commit 21a6f3a into NixOS:master Sep 21, 2020

oxalica mentioned this pull request Sep 21, 2020

vscode-extensions.vadimcn.vscode-lldb: init at 1.5.3 #91250

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rust-analyzer: 2020-08-24 -> 2020-09-14 and some clean-up #97444

rust-analyzer: 2020-08-24 -> 2020-09-14 and some clean-up #97444

oxalica commented Sep 8, 2020

danieldk commented Sep 8, 2020 •

edited

midchildan Sep 14, 2020

midchildan Sep 14, 2020

midchildan Sep 14, 2020

midchildan Sep 14, 2020

oxalica Sep 14, 2020

midchildan Sep 14, 2020 •

edited

oxalica Sep 15, 2020

midchildan Sep 15, 2020

midchildan Sep 15, 2020

oxalica Sep 15, 2020

midchildan Sep 15, 2020

midchildan left a comment

midchildan commented Sep 15, 2020

oxalica commented Sep 16, 2020

oxalica commented Sep 17, 2020

midchildan commented Sep 18, 2020 •

edited

oxalica commented Sep 18, 2020

midchildan commented Sep 18, 2020

jonringer left a comment


		nativeBuildInputs = lib.optionals doCheck [ rustPlatform.rustcSrc ];
		nativeBuildInputs = lib.optional useMimalloc cmake;

rust-analyzer: 2020-08-24 -> 2020-09-14 and some clean-up #97444

rust-analyzer: 2020-08-24 -> 2020-09-14 and some clean-up #97444

Conversation

oxalica commented Sep 8, 2020

Motivation for this change

Things done

danieldk commented Sep 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

midchildan Sep 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

midchildan left a comment

Choose a reason for hiding this comment

midchildan commented Sep 15, 2020

oxalica commented Sep 16, 2020

oxalica commented Sep 17, 2020

midchildan commented Sep 18, 2020 • edited

oxalica commented Sep 18, 2020

midchildan commented Sep 18, 2020

jonringer left a comment

Choose a reason for hiding this comment

danieldk commented Sep 8, 2020 •

edited

midchildan Sep 14, 2020 •

edited

midchildan commented Sep 18, 2020 •

edited