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

Allow optional installation of rust-docs component #253

Closed
wants to merge 2 commits into from
Closed

Allow optional installation of rust-docs component #253

wants to merge 2 commits into from

Conversation

cpcloud
Copy link

@cpcloud cpcloud commented Mar 14, 2021

This PR allows opting out of installing the rust-docs
component.

It can be used (for example) as follows, to create :

let
  tarballPath = https://github.com/cpcloud/nixpkgs-mozilla/archive/install-docs-optional.tar.gz;
  moz_overlay = import (builtins.fetchTarball tarballPath);
  pkgs = import <nixpkgs> { overlays = [ moz_overlay ]; };
in
pkgs.mkShell {
  name = "rust-shell-no-rust-docs";
  buildInputs = [
    (pkgs.rustChannelOf {
      channel = "beta";
      installDoc = false;
    })
  ];
}

Closes #252.
@miangraham
Copy link

I'm vendoring cpcloud's fork because it makes my CI builds stop crying due to unnecessary doc installation adding 10 minutes or so to each run. Wish I had more upvotes to give. Please merge.
@cpcloud
Copy link
Author

cpcloud commented Mar 28, 2021

@miangraham You may want to look into using fenix. I've been using it in https://github.com/cpcloud/turbocheck, and I'm liking it quite a bit.
nbp
nbp requested changes Aug 3, 2021
View reviewed changes
Copy link
Collaborator

@nbp nbp left a comment

Choose a reason for hiding this comment

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

Thanks, this change looks good except for the fact we should parametrize installDoc at the package level instead of the manifest file. (see comment below)

Also, I think most users of this overlay are likely feeling the same burden. Thus making the documentation copy default to false would be a good idea.
rust-overlay.nix Outdated
@@ -253,7 +253,7 @@ let
# All extensions in this list will be installed for the target architectures.
# *Attention* If you want to install an extension like rust-src, that has no fixed architecture (arch *),
# you will need to specify this extension in the extensions options or it will not be installed!
fromManifestFile = manifest: { stdenv, fetchurl, patchelf }:
fromManifestFile = manifest: { stdenv, fetchurl, patchelf, installDoc }:
Copy link
Collaborator

Choose a reason for hiding this comment

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

note: installDoc has no reason to bubble up to the processing of the Manifest file. A few lines below, there is makeOverridable ({extensions, targets, targetExtensions}: …) which is use to wrap each package in a similar way as callPackage which makes it usable with rust.override { extensions = ["rust-src"]; }.

This would be the place where to add installDoc ? false, such that it can be enabled by user who wants the doc to be installed.
@cpcloud
Copy link
Author

cpcloud commented Sep 5, 2021

@nbp I believe I have addressed your comments.
@cpcloud cpcloud requested a review from nbp September 5, 2021 13:00
nbp
nbp requested changes Sep 13, 2021
View reviewed changes
Copy link
Collaborator

@nbp nbp left a comment

Choose a reason for hiding this comment

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

Thanks for making these modifications, but I presume you might have noticed some issues or forgot to commit all changes.
rust-overlay.nix
Comment on lines -317 to +319
{ inherit (self) stdenv lib fetchurl patchelf; }
;
{
inherit (self) stdenv fetchurl patchelf;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

This revert a change from a previous patch which should cause this patch to not work as expected.
rust-overlay.nix
Comment on lines -134 to 138
installComponents = stdenv: namesAndSrcs:
installComponents = { stdenv, namesAndSrcs, installDoc }:
let
inherit (builtins) map;
installComponent = name: src:
installComponent = installDoc: { name, src }:
stdenv.mkDerivation {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: There is no need to add installDoc as argument of installComponent (no s), as it would be captured like stdenv is.
@cpcloud
Copy link
Author

cpcloud commented Dec 20, 2021

Closing this out. Feel free to revisit elsewhere. The review process took way too long and I don't have time to work on this anymore.
@cpcloud cpcloud closed this Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants
@cpcloud @miangraham @nbp