Nixpkgs: boost headers: not recognized as "system"

[pschuprikov]

Issue description

Compilation of a file that depends on _Boost_ using clang++ -Weverything -Werror ... results in an error:

include/boost/cstdint.hpp:33:11: error: macro name is a reserved identifier [-Werror,-Wreserved-id-macro]

The reason is the presence of #line preprocessor directive which essentially changes the effective file name, and it is no longer recognized as a system header. The issue was introduced in 83cecbdcbcf1fa2d933a4bd2925e086bb7972008.

Steps to reproduce

main.cpp:

#include <boost/cstdint.hpp>

int main() {}

shell.nix:

with import <nixpkgs>{}; {
  env-with-boost = stdenv.mkDerivation { 
    name = "env-with-boost"; buildInputs = [ boost ]; 
  };
}

Run:

nix-shell
clang++ -Weverything -Werror main.cpp

Technical details

• System: NixOS 16.03.769.f98d57b (Emu)
• Nix version: nix-env (Nix) 1.11.2
• Nixpkgs version: 16.03.895.63b9785

[AtnNn]

I'm seeing the same issue with GCC. When using boost and -Werror, I get errors like this one:

include/boost/date_time/gregorian/conversion.hpp:31:16: error: enumeration value ‘min_date_time’ not handled in switch [-Werror=switch-enum]

My IDE also gets confused by the non-existing include/boost. It would be nicer to have full paths to the boost headers in the error message.

@wkennington why were the #line directives added?

[vcunat]

Why: the full paths in assertion messages were introducing a runtime dependency on the headers.

[pschuprikov]

@vcunat The problem with the previous (before 83cecbdcbcf1fa2d933a4bd2925e086bb7972008) approach was that it only eliminated dependency between outputs of boost itself, so other boost-dependent packages were still dependent on boost-dev? Am I also correct, that it is basically a problem of any library that can be used in a header-only way?

Honestly, I'm uncomfortable with the current solution (e.g., debugger also gets confused), and I would like to investigate other approaches to avoid extra runtime dependencies.

[vcunat]

Yes, IIRC there was some dependency from within KDE packages, for example. Being header-only is neither required and nor enough to trigger this dependency. It's about the way __FILE__ is used in the header.

We could patch the __FILE__ occurrences, for example, but I see no easy solution ATM (the current sed there is simple).

[AtnNn]

I think this might be a bug in clang. I've opened an issue upstream https://llvm.org/bugs/show_bug.cgi?id=30752

[pschuprikov]

Let's see. Last time I asked on Clang's IRC, they answered that it was an intended behavior.

[pschuprikov]

What about doing something similar to what is suggested here? E.g., add #define __NIX_FILE__ that drops /nix/store/.../ part of the path, and then replace all the occurrences of __FILE__ with __NIX_FILE__ in boost headers. Surely, this is more invasive approach, but it lacks the annoying side effect.

Another alternative is to make the #line fixup an optional default, so that it could be disabled in the development environment.

[vcunat]

Hmm, yes, that sounds very good to me; I see no catch ATM. We should also refactor it into a setup hook that fixes all headers automatically (when put into build inputs), so it's easily reusable.

[vcunat]

(To clarify, I meant for the script to hard-code the shortened path, as strchr or similar might still leave the unshortened literal in the output.)

[pschuprikov]

It seems that the solution suggested previously is not viable. I skimmed through the __FILE__ occurrences in boost headers, and realized that there are a lot of macro definitions using __FILE__, some of which are to be used outside of boost. Thus, if we substitute __FILE__ with __NIX_FILE__ (defined in every boost header) and someone later uses it in client code (which doesn't know about __NIX_FILE__), abort messages will become really confusing.

[vcunat]

If we add #define __NIX_FILE__ "some/path" into those headers, e.g. boost ones, such problem won't be possible. It's possible that the substitutions happen on more places (in the header) than it would be ideal, but that seems unlikely to cause real problems.

[vcunat]

OMG, boost uses

#include __FILE__

[pschuprikov]

Yes=) I noticed this when I were testing the changes. I made an accidental typo and started getting something like ___NIX_FILE__ is not file in some boost PP file.

[vcunat]

Well, it's just a special case to handle. That self-inclusion was probably broken now. I'm now testing a version that should work fine, I think.

[vcunat]

I think I'll go with this commit: 2782d80, but I welcome suggestions/comments; the new file contains a lengthy explanation.

[pschuprikov]

I have a suggestion in a form of a commit that I was going to turn into a pull request, but I haven't yet tested it well: https://github.com/pschuprikov/nixpkgs/commit/3c36dd98daf3c85dddb50edf093cccd44f4e9d08.

I tried to keep assert messages as intact as possible taking into account that:

• macro containing __FILE__ can be used outside the file where it was defined
• after preprocessor leaves the file, the value of __NIX_FILE__ has to be restored (this is relevant only for #define __NIX_FILE__ approach.
• during development I might want to use e.g., BOOST_ASSERT, thus I need __NIX_FILE__ to be defined.

[vcunat]

Hmm, I see, that should be more correct with macros. I'm not certain whether the advantage is worth the extra __NIX_FILE__ hassle. There seems to be a slight risk that some code using the macro won't see any __NIX_FILE__ value at all.

Neither of these approaches solve the assert calls we have in xorg headers, as I'm very reluctant to touch such common stuff as assert.h, but I suppose we can ignore that case (leaving the #line hack in those).

[pschuprikov]

Under what circumstance a code won't see __NIX_FILE__ macro? I thought that adding -D__NIX_FILE__=__FILE__ to NIX_CFLAGS_COMPILE in a setup-hook should be enough.

I see no easy way to deal with xorg headers either (except potentially dangerous __FILE__ redefinition). Also, I don't really like the complexity of the solution.

Why can't we instead apply the solution that was used before #line introduction? What if we make it a setup-hook instead (extended to lib) so that it is applied to all dependent packages? Doing so we avoid any changes in header files, and we will be fixing the exact cause of the problem: the extra /nix/store/ references in libraries and binaries.

[pschuprikov]

Oh, sorry, I've just found the thread 09d970ce129146eb91158cfac6a96fcd95a2ac25 discussing the cons of the old solution.

[vcunat]

Patching binaries is more risky, as there might be some other legitimate references.

I see no direct likely circumstances ATM. nix-shell hopefully does source setup hooks. But many things are possible, e.g. for indirect dependencies... where a library might add -I${boost} to the *.pc file or similar mechanism but we didn't have boost in propagatedBuildInputs.

_(Some contributors actually prefer to export library dependencies only in .pc instead of fully propagating them, because that would allow other packages detect them even if they weren't explicitly specified.)_

[pschuprikov]

@vcunat how do we proceed? Do you commit your version, or do I submit a pull request (probably, better commented)?

I think, that your solutions is more reliable in the sense that it doesn't require any further hooks for packages dependent on boost. I doubt that regular users will pay much attention to assert messages anyway, so the only disadvantage is for developers willing to use boost test or assert libraries. For those cases I suggest to make it possible to turn the fixup hook off.

[vcunat]

Honestly, I don't know (yet).

[vcunat]

(I'm sorry for that overlong delay.)

@pschuprikov: your approach seems better to me, even if more complicated; are you (still) interested in finishing and testing it? (I assume you have some boost-dependent project.) I suppose at least self-inclusion still needs to be fixed up, e.g. in a way similar to my commit.

[pschuprikov]

Yes, I am still interested! I will try to test it this weekend.

[vcunat]

@pschuprikov: status/plans?

I wonder if we should e.g. use that working solution of mine in the meantime, even if slightly worse in some respects.

[pschuprikov]

Sorry, I didn't have much free time last days, and there was nothing wrong
with NixOS to bring me back to the context=)

I'm really going to do the testing this weekend, and then write back about
the status.

[pschuprikov]

I tried today both solutions, and it became apparent to me that none of the solutions is complete. This is due to the behavior of BOOST_ASSERT (doc), which expands to assert from assert.h.

Thus, I see three options here:

• Define NDEBUG (or BOOST_DISABLE_ASSERTS) so that assert macro does nothing. This may cause some programs using assert to stop terminating.
• Leave as is (with #line).
• Redefine __FILE__ in a way similar to what I have suggested for __NIX_FILE__.

[vcunat]

I believe compiling with NDEBUG is standard for production code in C(++), even if providing (split) debug symbols. I don't think we need to care that much about extra dependencies in "debugging builds".

[AtnNn]

This may be fixed in clang svn: https://bugs.llvm.org/show_bug.cgi?id=30752#c5

[mmahut]

Are there any updates on this issue, please

[knedlsepp]

This does currently also break coverage reports using gcov/lcov/gcovr where boost is a dependency, because gcov thinks it needs to search for include/boost in the local directory instead of in the nix-store.

[stale[bot]]

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

1. Search for maintainers and people that previously touched the
   related code and @ mention them in a comment.
2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
   irc.freenode.net.

[acairncross]

I'm still having the same issue as @knedlsepp