Dedupe more includeDirs and extraLibDirs

[nh2]

See NixOS/nixpkgs#41340.

Also see #3974 and #4105.

• Patches conform to the coding conventions.
• Any changes that could be relevant to users have been recorded in the changelog.
• The documentation has been updated, if necessary (not necessary).
• If the change is docs-only, [ci skip] is used to avoid triggering the build bots (it is not)

Haven't tested it yet, will do that tomorrow.

TODO

• test that it works for @nh2's use case
• [ ] investigate whether it's actually legitimate that cabal tries to combine the flags from these different sections into one (ordNubed or not) -- maybe there might be flags across the different sections that are mutually exculsive filed Check if cabal's cross-section combination of flags passed to GCC is legitimate #5360 instead

[hvr]

I'm not happy with the use of ordNub which changes the ordering of such order-sensitive CLI flags. Can we use a order-preserving nub?

nevermind, I realised that our ordNub does in fact retain the order (it retains the first occurence, like nub would)

[nh2]

I first thought this dedupe is only necessary if the outside world passes in duplicated flags.

But in fact this seems to fix the issue that before the patch, Cabal added a duplicated -L flag for each executable, test-suite and benchmark in the .cabal file.

I think we should also investigate whether it's actually legitimate that cabal tries to combine the flags from these different sections into one (ordNubed or not) -- maybe there might be flags across the different sections that are mutually exculsive.

[nh2]

What's up with that one Travis failure? Doesn't seem related to my PR.

• test that it works for @nh2's use case

I have now done that, works fine and I can build stuff again thanks to these ordNubs.

Regarding

• investigate whether it's actually legitimate that cabal tries to combine the flags from these different sections into one (ordNubed or not) -- maybe there might be flags across the different sections that are mutually exculsive

I've filed the separte issue #5360 instead as that isn't really related to this PR.

So from my side this is ready to merge.

[quasicomputational]

Patch looks sensible to me. The 7.10 builders are known to be flaky.

Does this solve the problem for good? It's a good, small patch and I think that it should be applied anyway, but the long-term solution would be response files if E2BIG is a problem, right?

[nh2]

Does this solve the problem for good?

@quasicomputational No, it only reduces the problem (length of the command line arguments) by factor N, where N is the number of executables+test suites+libraries+benchmarks you have in your .cabal file.

In practice for most projects now, it solves the problem, but if you make a project with truly many dependencies, it can still happen.

the long-term solution would be response files if E2BIG is a problem, right?

No, response files don't help, see my post here:

Note that even flags given in a "response file" via gcc @myresponsefile.rsp (which was designed to pass GCC flags via a file instead of command line args to circumvent command line arg limits) will be put into COLLECT_GCC_OPTIONS by gcc itself to communicate them to cc1 (I have just confirmed that with a small example on my Ubuntu 16.04). So using @myresponsefile.rsp is not a workaround. (Yes, this seems to defeat the purpose of response files, but I suspect those were originally made to circumvent a much smaller limit of command line argument on Windows, where the limit is well below the 128 KB limit for environment variable lengths on Linux).

[nh2]

Rebased to fix README conflict

[23Skidoo]

Someone should patch GCC to make it pass args to its subprocesses via response files.

[colonelpanic8]

Another thing to consider is that I believe that this needs to be fixed separately in e.g. hsc2hs. I know that NixOS/nixpkgs#40013 is caused by the call out to hsc2hs, and this fix does nothing to fix that.

[angerman]

I agree with @hvr that ordNub might be concerning. However for -L I don’t see any issue. For -I It might potentially break stuff like #include_next?

[nh2]

I agree with @hvr that ordNub might be concerning.

@angerman Wait, why? He retracted his concern above with

nevermind, I realised that our ordNub does in fact retain the order

which is correct. Or did I miss something?

[angerman]

I agree with @hvr that ordNub might be concerning.

@angerman Wait, why? He retracted his concern above with

I failed to see the retraction on mobile. Anyhow, even if we retain the order, we remove duplicate items later, which might interfere with #include_next.

However, I'm willing to just risk it, and figure out a work around in case something does some stupid #incude_next stuff. Maybe reordering of the includes would be enough.

[nh2]

@angerman Ah, I see.

It seems to me that this is not a big deal, because #include_next would break anyway in the presence of duplicate -I paths (as far as I can tell, a -I /my/path -I /my/path duplicate would break the feature).

We may be able to remove the deduplication if as part of #5360 we change the configure step to run one compile per executable/library/etc. section (because then, there may not be any Cabal-introduced replicates anyway), so we have a fallback solution if somebody really uses #include_next and it fails because of this change.

Further, I'm not sure if it's worth supporting; as per the documentation you linked,

There is no way to solve this problem within the C standard, but you can use the GNU extension #include_next
...
The use of #include_next can lead to great confusion. We recommend it be used only when there is no other alternative

so it wouldn't work e.g. with clang.

If we want to support it, it might be easier to just add an extra flag to cabal that says "don't do any path deduplication"; this would then be used in the rarest of cases, and in the default case, cabal wouldn't fail due to large amount of duplicated arguments.

I have just scanned all contents of Hackage (from my download at 2017-07-22), and there is no single use of #include_next. Of course that doesn't mean that it isn't used in some system .h file, but the chances of that are pretty low given how unportable that would be.

So at this stage, I recommend we deduplicate, and if somebody comes and actually needs this GNU-only feature, we add a cabal flag for them that they can use on their package in order to not deduplicate.

[angerman]

Let's do this.

[23Skidoo]

Looks OK. Do you think we can change the type of these fields to NubList instead of using ordNub?

[nh2]

I resolved the conflicts with github's resolution tool; what a bad idea, now I have a merge commit in my branch. One moment, will fix.

[nh2]

Looks OK. Do you think we can change the type of these fields to NubList instead of using ordNub?

@23Skidoo possibly, but it would be nice if you could do it in a separate commit, as right now other code in that file also uses ordNub and I have tested this change reasonably thoroughly already.

[dcoutts]

@nh2 said:

But in fact this seems to fix the issue that before the patch, Cabal added a duplicated -L flag for each executable, test-suite and benchmark in the .cabal file.

I think we should also investigate whether it's actually legitimate that cabal tries to combine the flags from these different sections into one (ordNubed or not) -- maybe there might be flags across the different sections that are mutually exculsive.

It's worth noting that this is only being done in checkForeignDeps as part of the various configure checks. Not that that makes it totally kosher, but it's not as if it's combining things for the real builds of the various components.

[23Skidoo]

This was green before I rebased, so merging straight away.

[23Skidoo]

@nh2 Thanks!