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

Reorganise include files #7847

Closed
wants to merge 23 commits into from
Closed

Conversation

andreabedini
Copy link
Contributor

Motivation

Currently the pkg-config files says to use the following include path

❯ pkg-config --cflags nix-main
-I/nix/store/6c14rkp59v85n9c29x69bllv7ijp9wya-nix-2.13.2-dev/include/nix -std=c++17 

I believe it would be more appropriate if this said

❯ pkg-config --cflags nix-main
-I/nix/store/6c14rkp59v85n9c29x69bllv7ijp9wya-nix-2.13.2-dev/include -std=c++17  # proposed

In this way consumers could do #include <nix/config.h>, while at the moment this does not work.

❯ g++ $(pkg-config --cflags nix-main) main.cc 
main.cc:1:10: fatal error: nix/config.h: No such file or directory
    1 | #include <nix/config.h>
      |          ^~~~~~~~~~~~~~
compilation terminated.

From what I can understand, in a stdenv pkg-config files are redundant because adding nix to your build dependencies automatically causes the compiler wrapper to pass -I${nix-dev}/include to the compiler, which is the same thing I believe we should do with pkg-config.

One example consumer of the nix libraries is hercules-ci-cnix-store, which cannot be compiled outside of a nix evironment (I should say without a nix wrapped compiler) because it includes the nix/config.h path.

~/code/hercules-ci-agent/hercules-ci-cnix-store
❯ rg nix/config
src/Hercules/CNix.hs
42:C.include "<nix/config.h>"

src/Hercules/CNix/Verbosity.hs
21:C.include "<nix/config.h>"

src/Hercules/CNix/Util.hs
28:C.include "<nix/config.h>"

src/Hercules/CNix/Store.hs
61:C.include "<nix/config.h>"

src/Hercules/CNix/Settings.hs
36:C.include "<nix/config.h>"

src/Hercules/CNix/Exception.hs
19:C.include "<nix/config.h>"

Context

I am making (so far very experimental) tools that link to nix code from outside of a nix stdenv. I have installed nix-dev in my profile and manually adjusted PKG_CONFIG_PATH.

❯ nix profile list | grep ^21
21 flake:nixpkgs#legacyPackages.x86_64-linux.nix github:NixOS/nixpkgs/8c619a1f3cedd16ea172146e30645e703d21bfc1#legacyPackages.x86_64-linux.nix /nix/store/6c14rkp59v85n9c29x69bllv7ijp9wya-nix-2.13.2-dev /nix/store/jifwi51d9sal3zql0n84xmabs3bk3v37-nix-2.13.2 /nix/store/vb7p3xsl06kks5zlkmwl5x9a73z7ncxz-nix-2.13.2-man

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

@andreabedini
Copy link
Contributor Author

I just noticed this is not enough because nix headers also don't have the nix/ in front so they are still not useable outside of the nix codebase. I'll have a go at changing this too (hopefully I won't get lost).

@edolstra
Copy link
Member

I just noticed this is not enough because nix headers also don't have the nix/ in front so they are still not useable outside of the nix codebase.

Yeah, this is the main reason we're not doing this already. I think it would require moving all the headers to a nix/ directory which would be quite annoying.

@Ericson2314
Copy link
Member

Thanks for doing this @andreabedini. It is also necessary to work on Hydra and Nix at the same time incrementally, which is critical for being able to address NixOS/hydra#1164 productively.

Yeah, this is the main reason we're not doing this already. I think it would require moving all the headers to a nix/ directory which would be quite annoying.

Git handles file moves very well. We should move the headers to /src/libnix*/nix subdirs and rip off the band-aid.

@andreabedini
Copy link
Contributor Author

Git handles file moves very well. We should move the headers to /src/libnix*/nix subdirs and rip off the band-aid.

I started working on this, although I opted for src/libutil/include/nix/util. The advantage being you can tell right away what component you are including. E.g.

-#include "serialise.hh"
+#include "nix/util/serialise.hh"

Let me know if you have strong opinions about this.

@Ericson2314
Copy link
Member

I personally like that (I am all about good layering!), and it shouldn't matter since this stuff is unstable anyways, but I will note that this is a breaking change as to where the files are installed whereas the other thing isn't.

@andreabedini
Copy link
Contributor Author

andreabedini commented Feb 17, 2023

I personally like that (I am all about good layering!), and it shouldn't matter since this stuff is unstable anyways, but I will note that this is a breaking change as to where the files are installed whereas the other thing isn't.

You are right, this was a step past the original intention which was only changing the pc files are readjusting the include paths. That would have only required a small change in the consumer build setup (e.g. a change in a Makefile) while this brings in (possibly annoying) a change in the source code. Thanks for pointing this out @Ericson2314!

I could add a bunch of symlinks to ease up the transition pain? (edit: or shims with a deprecation warning?)

@andreabedini andreabedini changed the title Better value for pkg-config cflags Reorganise include files Feb 17, 2023
@andreabedini andreabedini marked this pull request as draft February 17, 2023 06:46
@andreabedini
Copy link
Contributor Author

Added compatibility shims as well.

~/code/nix-tests $ make
g++ -I/nix/store/wmsdakkg2a6ndi8hy6faxdi191cvm7a9-nix-2.14.0pre20230220_a205add-dev/include -std=c++2a   -L/nix/store/ni1nk8dhd11f5zw4rb40kkb53pb62vyj-nix-2.14.0pre20230220_a205add/lib -lnixexpr -lnixstore -lnixutil -latomic_ops -lgc -lpthread -ldl   tmp.cpp   -o tmp
In file included from tmp.cpp:2:
/nix/store/wmsdakkg2a6ndi8hy6faxdi191cvm7a9-nix-2.14.0pre20230220_a205add-dev/include/nix/eval.hh:1:2: warning: #warning "Including nix/eval.hh is deprecated. Please include nix/expr/eval.hh instead." [-Wcpp]
    1 | #warning "Including nix/eval.hh is deprecated. Please include nix/expr/eval.hh instead."
      |  ^~~~~~~
In file included from tmp.cpp:3:
/nix/store/wmsdakkg2a6ndi8hy6faxdi191cvm7a9-nix-2.14.0pre20230220_a205add-dev/include/nix/util.hh:1:2: warning: #warning "Including nix/util.hh is deprecated. Please include nix/util/util.hh instead." [-Wcpp]
    1 | #warning "Including nix/util.hh is deprecated. Please include nix/util/util.hh instead."
      |  ^~~~~~~

@andreabedini andreabedini marked this pull request as ready for review February 20, 2023 09:06
Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

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

I really like the overall change (modulo a couple of small things). Given the amount of code churn, I'd like to leave @NixOS/nix-team a chance to voice their opinion, but it's a 👍 from me.

configure.ac Outdated Show resolved Hide resolved
@@ -0,0 +1,2 @@
#warning "Including nix/command.hh is deprecated. Include nix/cmd/command.hh instead."
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to generate these files on-the-fly rather than checking them out?

(they aren't big nor anything, but I imagine them being a bit confusing and very annoying when browsing the codebase with a fuzzy-finder)

Copy link
Member

@Ericson2314 Ericson2314 Feb 20, 2023

Choose a reason for hiding this comment

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

I personally don't think these stub files are worth the trouble. It's not too hard to do the bulk rename with a small script to see which library it was moved to. This stuff is explicitly unstable/internal, so breakage is not so much of an issue.

Copy link
Member

Choose a reason for hiding this comment

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

As a maintainer of a package that uses the headers headers, I will have to add a version check conditional around my imports anyway.

Automating this guidance is probably not worth the effort for our limited ecosystem of library consumers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If that's the consensus, I am happy to remove the shims. @roberth do you see any way we can make the transition easier for downstream consumers?

I think the worst part is figuring out where each include file went, and the shims addressed this by mentioning the location of the new file. I could make a awk script to migrate to the new locations but it won't help if you need to support both.

@edolstra
Copy link
Member

edolstra commented Feb 21, 2023

I'm not really in favor of this, not just because of the massive amount of code churn, but also because I rather like having the header files next to their corresponding implementation files (e.g. foo.{cc,hh}).

Also, the resulting paths seem superfluous/repetitive (e.g. nix/src/libcmd/include/nix/cmd).

@Ericson2314
Copy link
Member

Ericson2314 commented Feb 21, 2023

I suggest that this PR #7847, PR #7870, and my old #5589 both touch on very similar issues, and projects like #7735 (more bindings) somewhat force the issue.

These PRs are hard to rebase (thought actually easy to rebase other PRs over! The disruption is not bad!) so I guest we figure out what we want in an issue first.

I'll get started on writing that issue.

@Ericson2314
Copy link
Member

#7876 This is the issue. Please comment there!

@fricklerhandwerk
Copy link
Contributor

fricklerhandwerk commented Feb 24, 2023

Triaged in the Nix team meeting:

  • @edolstra: this PR changes essentially every source file to effectively better interop with pkg-config
    • don't see how the cost justifies the advantage
    • @Ericson2314: this unlocks some work down the line, would like to discuss this
  • @thufschmitt: should keep this low priority unless something important depends on this
  • to discuss

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-02-24-nix-team-meeting-minutes-35/25757/1

@andreabedini
Copy link
Contributor Author

I am aware this change is quite invasive. pkg-config by itself is not the goal, the goal is to make it easier to link to nix C++ API from other projects (e.g. outside nix). Language bindings come to mind but the sky is the limit.

@Ericson2314
Copy link
Member

@andreabedini I will say the notes reflect but a very brief triaging conversation moving this to the "to discuss column".

@thufschmitt thufschmitt added the contributor-experience Developer experience for Nix contributors label Feb 27, 2023
@fricklerhandwerk
Copy link
Contributor

Discussed in Nix team meeting:

@andreabedini we decided to postpone this for now, as the change is fairly costly in terms of moving around lots of files. We will reconsider it in light of the more general issue when we make more progress there: #7876

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-03-27-nix-team-meeting-minutes-44/26759/1

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-04-14-nix-team-meeting-minutes-48/27358/1

@fricklerhandwerk
Copy link
Contributor

Revisited in the Nix maintainers meeting:

  • @roberth: counter-proposal: keep headers next to source files, and almost in their existing locations:
    • instead of include/nix/store/*.hh (this PR) or src/libstore/*.hh (status quo) use src/libnixstore/*.hh (note the nix)
    • then a source build would be -I./src (new) and for consumers -I${nix}/include (like this PR already)
    • In both cases, #include "libnixexpr/*.hh" works
    • the alternative of putting in place symlinks as a simulation of the old structure is probably tech debt, as it will complicate dealing with renames in Git
      • @edolstra: also we break the API all the time and never made guarantees about it
  • @thufschmitt: another hacky solution would be patching header files on installation
  • we agree on @roberth's proposal

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-07-21-nix-team-meeting-minutes-73/30768/1

@Ericson2314
Copy link
Member

Ericson2314 commented Jul 23, 2023

Oh one downside to -I./src is that it would expose all headers to all libraries, allowing mistakes where upstream libraries could use downstream libraries' headers.

I think @thufschmitt had started to think of symlinking so we could use the layout from the issue while still keeping the headers next to sources as @edolstra wanted? That would also be a good option.

@roberth
Copy link
Member

roberth commented Jul 23, 2023

Oh one downside to -I./src is that it would expose all headers to all libraries, allowing mistakes where upstream libraries could use downstream libraries' headers.

Another way to check that is to have a derivation per library. That would save some rebuilds as well.

@andreabedini
Copy link
Contributor Author

At this point I would say the PR is useful only for the discussion it generated. Given this PR is also mentioned in the discussion in #7876, I can be confident it will not get lost and close it ☺️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor-experience Developer experience for Nix contributors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants