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.

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

Add cargo features #1197

Closed

Conversation

vdagonneau-anssi
Copy link
Contributor

Hello !

I have been using nix for a while now across my projects and I consistently use only small subsets of it. I therefore modified nix in order to use features so I can only "pay" for what I use.

The main advantage is shorter compilation times, it also allows for reducing transitive dependencies (void for example is only used by reboot and unistd).

@vdagonneau-anssi vdagonneau-anssi changed the title Modified nix to use features Add cargo features Mar 19, 2020
@asomers
Copy link
Member

asomers commented Mar 27, 2020

Thanks for your hard work. Something like this has been dimly on the radar for a long time (see
#190). Everybody can see the benefits, but nobody has done the necessary hard work. I see you have a working PR; that's more than anybody else has done. But there are still some important questions remaining:

  1. With this PR, does Nix still compile with every combination of options? Verifying that would seem difficult.
  2. How does Cargo handle a crate that is pulled in from multiple points with different options? For example, if crate top depends on foo, bar, and baz, and foo depends on nix { features = ["signal"]}, bar depends on nix { features = ["aio"] }, and baz depends on nix { features = ["eventfd"]}, what will happen?
  3. Overall, is this approach better or worse than using subcrates?

@vdagonneau-anssi
Copy link
Contributor Author

Thanks for your hard work. Something like this has been dimly on the radar for
a long time (see #190). Everybody can see the benefits, but nobody has done
the necessary hard work. I see you have a working PR; that's more than anybody
else has done. But there are still some important questions remaining:

I think the entire Rust ecosystem is currently trying to figure out how to
properly use cargo features. It is undoubtedly powerful but trick to get right.
My current PR is a naive first-draft at trying it for Nix.

My idea was to divide nix in a few big subsystems. I do not have usage
statistics for nix and you might have better information than me on that front
but I think some (most?) users of nix are only using one or two subsystem at the
time.

The current PR basically puts every syscall category in its own feature but I
would be interested to hear any proposal for less granular subsystems.

1. With this PR, does Nix still compile with every combination of options?
   Verifying that would seem difficult.

That is a very valid question; One I have been thinking about.

I think it is very clear that the combinatorics are not on our side here.
Basically, even with a small amount (>4 or >5) of subsystems, we cannot
guarantee that every single possible combination will work.

I do not think we have to make that guarantee though. We can probably test a
default feature containing all of the features, much like the current PR. And
then test all of the independent features, one at a time. That would give us a
reasonable expectation that everything works.

Additionally, I think deactivating features in nix should be only advertised to
advanced users with the caveat that they need to make sure that the exact
combination they are using is working properly.

2. How does Cargo handle a crate that is pulled in from multiple points
   with different options?  For example, if crate `top` depends on `foo`,
   `bar`, and `baz`, and `foo` depends on `nix { features = ["signal"]}`,
   `bar` depends on `nix { features = ["aio"] }`, and `baz` depends on
   `nix { features = ["eventfd"]}`, what will happen?

I just checked that. Cargo seems to be doing the reasonable thing here. Only one
nix crate will be built in that situation with an aggregate of all the activated
features.

In your example, nix is build with features aio, eventfd and signal.

3. Overall, is this approach better or worse than using subcrates?

I feel like features are better than subcrates but that is a personal opinion
not based on any strong technical reason.

In any way, I do not think it is any worse than subcrates in the case of nix.

@vdagonneau-anssi
Copy link
Contributor Author

For reference, here is a dependency graph of the current features I introduced.

graphviz

After some regrouping, I think we could do something like that:

graphviz-after

sources for the graphs
digraph G {
    overlap = false;
    
    aio -> signal;
    _dir -> fcntl, stat;
    env ;
    epoll ;
    event ;
    eventfd ;
    fcntl -> uio, stat;
    features -> utsname;
    ifaddrs -> net, socket;
    inotify -> unistd;
    ioctl ;
    kmod ;
    memfd ;
    mman -> stat, fcntl;
    mount ;
    mqueue -> stat, fcntl;
    net ;
    poll -> time, signal;
    pthread ;
    ptrace -> unistd, signal;
    pty -> termios, fcntl, unistd;
    quota ;
    reboot -> void;
    sched -> unistd;
    select -> signal,time;
    sendfile ;
    signalfd -> signal;
    signal -> unistd;
    socket -> ifaddrs, uio, time;
    statfs ;
    stat -> time, fcntl;
    statvfs ;
    termios -> unistd;
    time ;
    ucontext -> signal;
    uio -> unistd;
    unistd -> void, fcntl;
    utsname ;
    wait -> signal;
}
digraph G {
    K = 1.5;
    subgraph cluster_network {
        ifaddrs;
        net;
        socket;
    
        color = green;
        label = "network";
    }
    
    ifaddrs -> {net, socket};
    socket -> {ifaddrs};
    cluster_network -> cluster_core [label = "uio,time"];
    
    subgraph cluster_core {
        fcntl;
        unistd;
        signal;
        time;
        uio;
        stat;
        
        color = blue;
        label = "core";
    }
    
    fcntl -> {uio, stat};
    unistd -> fcntl;
    signal -> unistd;
    uio -> unistd;
    stat -> {time, fcntl};
    cluster_core -> void;
    
    subgraph cluster_event {
        inotify;
        select;
        poll;
        epoll;
        
        color = purple;
        label = "event";
    }
    
    cluster_event -> cluster_core [label = "unistd,signal,time"];
    
    subgraph cluster_fs {
        statvfs;
        mount;
        statfs;
        dir;
        
        color = yellow;
        label = "fs";
    }
    
    cluster_fs -> cluster_core [label = "fcntl,stat"];
    
    subgraph cluster_terminal {
        termios;
        pty;
        
        color = red;
        label = "terminal";
    }
    
    pty -> termios;
    cluster_terminal -> cluster_core [label = "fcntl, unistd"];
    
    subgraph cluster_ipc {
        mqueue;
        
        color = orange;
        label = "IPC";
    }
    
    cluster_ipc -> cluster_core [label = "stat,fcntl"];
    
    aio -> cluster_core [label = "signal"];
    env -> {};
    event -> {};
    eventfd -> {};
    features -> utsname;
    ioctl -> {};
    kmod -> {};
    memfd -> {};
    mman -> cluster_core [label = "stat,fcntl",len="5"];
    mount -> {};
    pthread -> {};
    ptrace -> cluster_core [label = "unistd,signal"];
    quota -> {};
    reboot -> void;
    sched -> cluster_core [label = "unistd"];
    sendfile -> {};
    signalfd -> cluster_core [label = "signal"];
    time -> {};
    ucontext -> cluster_core [label = "signal"];
    utsname -> {};
    wait -> cluster_core [label = "signal"];
}

@asomers
Copy link
Member

asomers commented Apr 1, 2020

Cool graph. How did you make it? Was that done with cargo-modules?

@vdagonneau-anssi
Copy link
Contributor Author

No, manually with just a little bit of scripting.

@glebpom
Copy link
Contributor

glebpom commented Apr 8, 2020

I just wanted to add that tokio 0.2 has successfully adopted the same scheme with features, and it works quite well.

@asomers
Copy link
Member

asomers commented Aug 22, 2021

@vdagonneau the reason I sort of dropped this PR was because I found it too daunting. But I want to get back to it now, especially because I just discovered cargo-hack, which makes it possible to actually ensure that the feature flags work.

Superseded by #1498

@asomers asomers closed this Aug 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants