Skip to content

CRAN R packages, rewritten#5078

Closed
taku0 wants to merge 11 commits intoNixOS:masterfrom
taku0:cran-packages
Closed

CRAN R packages, rewritten#5078
taku0 wants to merge 11 commits intoNixOS:masterfrom
taku0:cran-packages

Conversation

@taku0
Copy link
Contributor

@taku0 taku0 commented Nov 22, 2014

Packages for CRAN, the Comprehensive R Archive Network.
I have managed to compile 5000+ packages and masked ~300 packages which cannot be installed due to lack of dependencies or other reasons.

The files are organized as follows:

  • cran-packages.nix: entry point
  • sources.nix: source file definitions, auto-generated
  • generate_sources.R: generator script for sources.nix
  • generic-builder.nix: builder script, R CMD INSTALL with appropriate options and environment variables
  • wrapper.nix: wrapper generator that combines R and CRAN packages
  • packagesWithNativeBuildInputs.nix: additional nativeBuildInputs definitions for native library dependencies
  • packagesWithBuildInputs.nix: additional buildInputs definitions for executable command dependencies
  • packagesRequireingX.nix: list of packages requireing X to be running while installation
  • packagesToSkipTests.nix: list of packages requireing special setup (e.g. MPI processes) for testing
  • defaultOverrides.nix: other ad-hoc tweaks
  • patches/: ad-hoc patches
  • maskedPackages.nix: list of packages which cannot be compiled

I have checked that all packages are compiled with following ~/.nixpkgs/config.nix.

{ 
  packageOverrides = pkgs: rec {
    rWrapper = pkgs.rWrapper.override {
      packages = map (name: builtins.getAttr name pkgs.rPackages) (builtins.attrNames pkgs.rPackages);
    };
  };

  ...
}

That does not mean that all packages do work properly; some packages may lack optional features, run slower than it can, or even worse crash with runtime error.

Note that Nix cannnot build a package with thousands of dependencies due to a limitation of shell script, so that building rWrapper with ~/.nixpkgs/config.nix above result in Argument list too long after building all dependencies.

Unfortunately, some packages may update source archives without changing version numbers, so that installation may fail with a hash verification error.

@peti peti self-assigned this Nov 23, 2014
@peti peti added the 0.kind: enhancement Add something new or improve an existing system. label Nov 23, 2014
@peti
Copy link
Member

peti commented Nov 23, 2014

Test builds are running at http://hydra.cryp.to/jobset/nixpkgs/r-packages.

@peti
Copy link
Member

peti commented Nov 23, 2014

The packages rpud and CARramps depend on the non-free NVidia drivers, which means we are not allowed to build (and distribute) them on the Hydra server. This is accomplished by setting the meta.hydraPlatforms attribute to none like so:

meta.hydraPlatforms = stdenv.lib.platforms.none;

I suppose this should be added in the overrides file?

@peti
Copy link
Member

peti commented Nov 23, 2014

The file name defaultOverrides.nix seems inconsistent with other file name is the r-modules directory because it uses camel caps rather than dashes. This should probably be called default-overrides.nix instead. The same applies to maskedPackages.nix, packagesRequiringX.nix, etc.

Also, the file generate_sources.R should use a hyphen instead of an underscore for consistency.

@peti
Copy link
Member

peti commented Nov 23, 2014

I cherry-picked the commit that adds udunits in 34e714c. If you have the chance, please rebase this pull request relative to the current master branch and force-push the result.

Copy link
Member

Choose a reason for hiding this comment

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

The configureFlags include --with-udunits, but the library isn't passed as a build input. I'm not sure whether that was done intentionally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not intentional. Building UDUNITS with older version of NetCDF required this option, but UDUNITS is now a separate package, so that this option is noop now. I will fix this.

@peti
Copy link
Member

peti commented Nov 23, 2014

The maskedPackages.nix file seems redundant. Nixpkgs usually adds at attribute meta.broken = true to packages that are known to fail to compile. IMHO adding that attribute via the overrides file would be preferable over having a separate file for this purpose.

@peti
Copy link
Member

peti commented Nov 23, 2014

What is the reason for splitting the package definitions into 6 different files: packagesWithNativeBuildInputs.nix, packagesWithBuildInputs.nix, packagesRequireingX.nix, packagesToSkipTests.nix, cran-packages.nix, and sources.nix! That doesn't seem like a structure that's particularly easy to navigate. When I -- as a maintainer -- want to edit the package Brobdingnag, then I may not know whether it requires X or not, etc., so I'll end up having to grep through all those files just to figure out where the expression is defined. IMHO, having two files -- the generated cran-packages.nix and the manually edited default-overrides.nix -- would be just fine. Additional structure feels a little over the top.

@taku0
Copy link
Contributor Author

taku0 commented Nov 24, 2014

The packages rpud and CARramps depend on the non-free NVidia drivers, which means we are not allowed to build (and distribute) them on the Hydra server. This is accomplished by setting the meta.hydraPlatforms attribute to none

I will fix them with overrides file.

This should probably be called default-overrides.nix instead. The same applies to maskedPackages.nix, packagesRequiringX.nix, etc.

I will fix them.

I cherry-picked the commit that adds udunits in 34e714c. If you have the chance, please rebase this pull request relative to the current master branch and force-push the result.

Yes.

The maskedPackages.nix file seems redundant. Nixpkgs usually adds at attribute meta.broken = true to packages that are known to fail to compile.

I didn't know that attribute. I will fix them.

IMHO, having two files -- the generated cran-packages.nix and the manually edited default-overrides.nix -- would be just fine.

I agree with you that six files are too many. How about three files, cran-packages.nix for hand-written logics, sources.nix for auto generated data, and default-overrides.nix for hand-written, messy definitions? If we mix generated data (sources.nix) with logics (cran-packages.nix), someone may edit the generated file rather than the generator script to fix the logic (e.g. 76fc080, where default.nix was generated by generate_nix.rb. Now the default.nix for thunderbird-bin is separated into logics and auto-generated data by #4829).

Thank you for your comments.

@taku0
Copy link
Contributor Author

taku0 commented Nov 24, 2014

I have rebased the branch. I will fix other parts.

@taku0
Copy link
Contributor Author

taku0 commented Nov 29, 2014

Updating sources.nix results in ~20 build errors. We may need a smarter generator, that recognize dependencies for native libraries and unsatisfiable packages. I will update the pull request anyway after fixing those errors.

@taku0
Copy link
Contributor Author

taku0 commented Nov 29, 2014

Updated sources.nix and fixed all issues.
Most errors ware the lack of X server. Perhaps we should propagate requireX attribute.

@peti
Copy link
Member

peti commented Nov 30, 2014

I don't like the name sources.nix much. It's too generic and doesn't convey any meaning. The name cran-packages.nix was superior, IMHO. Can't we get along with two files instead of three, i.e. default.nix and cran-packages.nix?

@taku0
Copy link
Contributor Author

taku0 commented Dec 1, 2014

Two packages, auto-generated one and hand-written one, is acceptable.
Combined cran-packages.nix and default-overrides.nix into default.nix and renamed sources.nix to cran-packages.nix.

@peti
Copy link
Member

peti commented Dec 3, 2014

Okay, I've merged this patch (plus a few minor edits) to master. Thank you very much for the effort you've put into this update, this is really a vast improvement over the current state! Test builds are again running at http://hydra.cryp.to/jobset/nixpkgs/r-packages.

@peti peti closed this Dec 3, 2014
@peti
Copy link
Member

peti commented Dec 4, 2014

@taku0, there is one more thing: I'm still worrying about the flock thing in our generic builder. Nix generally runs those builds in a specifically set-up chroot environment, and it's hard to see for me how multiple xvfb-run instances running in parallel could possibly affect each other. What exactly is the problem that you were experiencing and why did the -a switch not alleviate it?

@taku0
Copy link
Contributor Author

taku0 commented Dec 4, 2014

The use of a chroot requires that Nix is run as root (see description for build-use-chroot option in Nix User's Guide).
xvfb-run with -a option allocates a fresh display number for the server. Unfortunately, allocation logic below simply scans lock files in /tmp but not create a lock file. Xvfb does create the lock file with given display number. If two processes try to allocate display numbers simultaneously, they result in the same display number. Xvfb fails if it cannot create a lock file.

# Find a free server number by looking at .X*-lock files in /tmp.
find_free_servernum() {
    # Sadly, the "local" keyword is not POSIX.  Leave the next line commented in
    # the hope Debian Policy eventually changes to allow it in /bin/sh scripts
    # anyway.
    #local i

    i=$SERVERNUM
    while [ -f /tmp/.X$i-lock ]; do
        i=$(($i + 1))
    done
    echo $i
}
...
SERVERNUM=$(find_free_servernum)
...
XAUTHORITY=$AUTHFILE Xvfb ":$SERVERNUM" $XVFBARGS $LISTENTCP >>"$ERRORFILE" \
  2>&1 &
...
DISPLAY=:$SERVERNUM XAUTHORITY=$AUTHFILE "$@" 2>&1

(from $(xvfb_run)/bin/.xvfb-run-wrapped)

I encountered several errors when I ran nix-env with -j 4 option before introducing the flock.

@peti
Copy link
Member

peti commented Dec 4, 2014

The use of a chroot requires that Nix is run as root.

Actually, it quite sufficient for nix-daemon to run as root. Normal
users can still run nix-env and take advantage of chroot builds.

xvfb-run with -a option allocates a fresh display number for the server. Unfortunately, allocation logic below simply scans lock files in /tmp but not create a lock file. Xvfb does create the lock file with given display number. If two processes try to allocate display numbers simultaneously, they result in the same display number.

Wouldn't it be best to report that kind of issue to the authors of xvfb-run so that they can fix the problem for the benefit of everyone? I mean, this sounds like a genuine bug in the implementation of the -a flag -- that's not a probem that's specific to Nix.

@peti
Copy link
Member

peti commented Dec 4, 2014

Filing an upstream bug report turns out to be surprisingly difficult. It's a little unclear who the canonical source of that package is, really.

IMHO, the flock call should wrap the find_free_servernum bit, but not the entire script. Then the -a option would do the right thing.

@taku0
Copy link
Contributor Author

taku0 commented Dec 5, 2014

Normal users can still run nix-env and take advantage of chroot builds.

Chrooting is optional anyway. We must make a package definition works in non-chroot environments.

IMHO, the flock call should wrap the find_free_servernum bit, but not the entire script. Then the -a option would do the right thing.

flocking find_free_servernum and creating a lock file (say /tmp/.xvfb_run$i.lock) inside the function is better than the current implementation and works for the cran-packages. If we could patch Xvfb to makef it choose a display number, it would be more robust.

@Mathnerd314
Copy link
Contributor

Filing an upstream bug report turns out to be surprisingly difficult. It's a little unclear who the canonical source of that package is, really.

The evidence I can find suggests that Debian is the canonical upstream; it was added by Branden Robinson in 2001, and every other version on the internet contains the "If anyone is using this to build a Debian package" line from that release. Unfortunately it seems Debian is not maintaining it as there are several ancient unreviewed patches in the bug tracker. But maybe someone will respond if a new bug report is filed.

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

Labels

0.kind: enhancement Add something new or improve an existing system.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants