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

Dedupe include dirs inherited from dependencies #3974

Merged
merged 1 commit into from
Oct 12, 2016

Conversation

niteria
Copy link
Collaborator

@niteria niteria commented Oct 11, 2016

If you build a big number of packages, all with
the same extra -I flags, the flags get inherited
by the dependent packages and duplicated.
For big dependency trees it can exceed the
maximum command line length on some systems,
this happened to me with Linux and hoogle 5.

This patch decreases the redundancy by
dropping all but the first occurrence of
an include dir, preserving the semantics,
as they are processed left to right.

@mention-bot
Copy link

@niteria, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dcoutts, @ezyang and @ttuegel to be potential reviewers.

Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

Good catch. Just a minor suggestion about code layout.

++ [ "-I" ++ dir
| dep <- deps
, dir <- Installed.includeDirs dep ]
++ installedIncludeDirs
Copy link
Contributor

@dcoutts dcoutts Oct 12, 2016

Choose a reason for hiding this comment

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

Personally I'd keep it inline here rather than pulling it out. e.g.

++ [ "-I" ++ dir
   , dir <- ordNub [ dir | dep <- deps
                         , dir <- Installed.includeDirs dep ]
            -- dedupe include dirs of dependencies
            -- to prevent quadratic blow-up
   ]

@niteria niteria force-pushed the dedupe-recursive-include-dirs branch 2 times, most recently from cd4fb25 to ca09443 Compare October 12, 2016 00:36
If you build a big number of packages, all with
the same extra -I flags, the flags get inherited
by the dependent packages and duplicated.
For big dependency trees it can exceed the
maximum command line length on some systems,
this happened to me with Linux and hoogle 5.

This patch decreases the redundancy by
dropping all but the first occurrence of
an include dir, preserving the semantics,
as they are processed left to right.
@niteria niteria force-pushed the dedupe-recursive-include-dirs branch from ca09443 to 6e31b30 Compare October 12, 2016 00:37
@niteria
Copy link
Collaborator Author

niteria commented Oct 12, 2016

I've updated to accommodate the suggestion with a minor formatting change to respect 80-col limit.

@23Skidoo
Copy link
Member

LGTM. Feel free to merge once Travis is green.

@niteria
Copy link
Collaborator Author

niteria commented Oct 12, 2016

Travis timed out, I've tried restarting the job but it didn't seem to have done anything.

@23Skidoo
Copy link
Member

I restarted it myself, seems to have worked.

@23Skidoo 23Skidoo merged commit 5345fcf into haskell:master Oct 12, 2016
@23Skidoo
Copy link
Member

Merged, thanks!

@nh2
Copy link
Member

nh2 commented Nov 12, 2016

@niteria I have a question about this. In the commit you say:

This patch decreases the redundancy by
dropping all but the first occurrence of
an include dir, preserving the semantics,
as they are processed left to right.

But if they are processed left to right (and thus I assume later ones override earlier ones) shouldn't you keep the last one instead the first?

E.g. if you have dirs a/ and b/ each including file.h, then -Ia/file.h -Ib/file.h -Ia/file.h would be different from -Ia/file.h -Ib/file.h, as in the former it's picked from a/ and the latter from b/?

@niteria
Copy link
Collaborator Author

niteria commented Nov 12, 2016

@nh2: Here's the documentation: https://gcc.gnu.org/onlinedocs/cpp/Search-Path.html

The relevant part:

You can add to this list with the -Idir command-line option. All the directories named by -I are searched, in left-to-right order, before the default directories. The only exception is when dir is already searched by default. In this case, the option is ignored and the search order for system directories remains unchanged.

My understanding is that it searches left to right and takes the first one that matches.
So in your example both situations would have the same behavior.

@nh2
Copy link
Member

nh2 commented Nov 13, 2016

OK, if that's how it works, then that's OK.

nh2 added a commit to nh2/cabal that referenced this pull request Jun 1, 2018
… and `PD.extraLibDirs`.

Should help with big invocations as found in
NixOS/nixpkgs#41340.
@nh2 nh2 mentioned this pull request Jun 1, 2018
5 tasks
nh2 added a commit to nh2/cabal that referenced this pull request Jun 1, 2018
… and `PD.extraLibDirs`.

Should help with big invocations as found in
NixOS/nixpkgs#41340.
colonelpanic8 pushed a commit to colonelpanic8/cabal that referenced this pull request Jun 4, 2018
… and `PD.extraLibDirs`.

Should help with big invocations as found in
NixOS/nixpkgs#41340.
nh2 added a commit to nh2/cabal that referenced this pull request Jun 10, 2018
… and `PD.extraLibDirs`.

Should help with big invocations as found in
NixOS/nixpkgs#41340.
nh2 added a commit to nh2/cabal that referenced this pull request Jun 14, 2018
… and `PD.extraLibDirs`.

Should help with big invocations as found in
NixOS/nixpkgs#41340.
23Skidoo pushed a commit to nh2/cabal that referenced this pull request Jun 20, 2018
… and `PD.extraLibDirs`.

Should help with big invocations as found in
NixOS/nixpkgs#41340.
23Skidoo pushed a commit that referenced this pull request Jun 20, 2018
…LibDirs`.

Should help with big invocations as found in
NixOS/nixpkgs#41340.
nh2 added a commit to nh2/cabal that referenced this pull request Jul 17, 2018
… and `PD.extraLibDirs`.

Should help with big invocations as found in
NixOS/nixpkgs#41340.
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.

5 participants