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

Should probably run with strictDeps = true #403

Closed
dpc opened this issue Sep 29, 2023 · 4 comments · Fixed by #430
Closed

Should probably run with strictDeps = true #403

dpc opened this issue Sep 29, 2023 · 4 comments · Fixed by #430

Comments

@dpc
Copy link
Contributor

dpc commented Sep 29, 2023

It is very easy to trigger needless rebuilds of dependencies between multiple cargo inheritance steps, by just introducing seamingly unrelated binary, only because by default mkDerivation seems to make dependencies of your build inputs also available in the derivation.

So I had a one off test, imported some extra binary in nativeBuildInputs just for that one step, and I see openssl-sys and downstream crates are rebuilding because of that. Turns out that binary was using a some library which is them being added to PKG_CONFIG_PATH, which makes build.rs of openssl-sys want to rebuild. Tricky to debug.

I was told on NixOS Matrix channel that using strictDeps = true; is generally a good idea, so I'm going to do just that, but crane could do that by default, given how it is used. Might save some users lots of trouble, at the minor cost of having to list all direct build inputs.

@ipetkov
Copy link
Owner

ipetkov commented Oct 1, 2023

Haven't used it myself before, what are the impacts of setting strictDeps? I'm assuming it will change what things are actually set in the various environment variables, but I'm not fully certain whether that might "break" existing derivations

FWIW adding a new nativeBuildInput will cause a rebuild anyway (since that input will show up as an environment variable to the derivation), though not including it in the pkg-config variables would avoid potentially invalidating build-script caches

@dpc
Copy link
Contributor Author

dpc commented Oct 1, 2023

I'm assuming it will change what things are actually set in the various environment variables

I'm not entirely sure. I've found:

StrictDeps enforces desirable properties like outputs not retaining references to build tools. It also prevents mistakes that break cross-compilation.

It seems to me that without it Nix mkDerivation's infers some inputs transitively.

FWIW adding a new nativeBuildInput will cause a rebuild anyway (since that input will show up as an environment variable to the derivation

Yes, but it will not trigger cargo rebuilding already built dependencies, which right now does happen. E.g. I build the whole workspace, inherit the resulting ./target into a test where I import some binary, and bam, I see the workspace being rebuilt again by cargo test only because that binary changed PKG_CONFIG_PATH for some reason.

whether that might "break" existing derivations

It does. I started using it and had to move certain things between buildInputs and nativeBuildInputs (pkg-config has to be in nativeBuildInputs or cargo will not find stuff). It worked just fine previously.

@ipetkov
Copy link
Owner

ipetkov commented Oct 2, 2023

Although I do agree that strictDeps is probably desirable to always have on going forward, I worry that this is a departure from being a thin wrapper around mkDerivation (i.e. I worry people will be confused why stuff doesn't work with mkCargoDerivation). Though maybe we could highlight it in all the examples as a start 🤔

@dpc
Copy link
Contributor Author

dpc commented Oct 2, 2023

I think you're right. It is confusing, especially when it happens implicitly.

tfkhim added a commit to tfkhim/sway-workspace-extras that referenced this issue Nov 1, 2023
It is generally recommended to use this option to avoid having
unnecessary runtime dependencies [1].

Links:
  [1] ipetkov/crane#403
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 a pull request may close this issue.

2 participants