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.

Sign up for GitHub

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

Jump to bottom

hsc2hs invocation picks up too many flags from pkgdb #2971

Closed
ezyang opened this issue Dec 21, 2015 · 3 comments · Fixed by #4033
Closed

hsc2hs invocation picks up too many flags from pkgdb #2971

ezyang opened this issue Dec 21, 2015 · 3 comments · Fixed by #4033
Labels
Cabal: other type: bug
Milestone
2.0

Comments

@ezyang
Copy link
Contributor

ezyang commented Dec 21, 2015

Cabal computes the transitive dependency of packages, and uses include-paths and other fields to determine what to pass to hsc2hs. But it grabs too many packages: it does the entire set of all packages that ANY component in the package might need, rather than the component that is being built. Test case attached. I'll push a comment to the code that needs to be fixed too.
test.zip

To run the test, first build p and q, and note that it fails. Now touch /tmp/asdf.h, and observe that it now succeeds. p adds /tmp to import path, but q's buildable executable does not refer to p and should not see it.
ezyang added a commit that referenced this issue Dec 21, 2015
@ezyang
Comment on #2971 at the relevant place needing a fix.
679812d 
Signed-off-by: Edward Z. Yang <[email protected]>
@ezyang
Copy link
Contributor Author

ezyang commented Jan 13, 2016

When #3022 is merged this should only take a local change to fix.

@ezyang ezyang mentioned this issue Jan 13, 2016
Closed
@ezyang
Copy link
Contributor Author

ezyang commented Aug 15, 2016

Actually, there is a bit more refactoring to do. The problem is the PackageIndex we store in packageDependsIndex doesn't contain the INTERNAL components we have, so it's inconvenient to ask for the closure of a given subcomponent; it will refer to things that are not in the index. Actually I think this reveals a bug, where if this won't build:

name:                PreProcessInternal
version:             0.1.0.0
license:             BSD3
author:              Edward Z. Yang
maintainer:          [email protected]
build-type:          Simple
cabal-version:       >=1.10

library
  build-depends:       base
  include-dirs:        include
  install-includes:    asdf.h
  default-language:    Haskell2010

executable exe
  main-is: Main.hs
  build-depends:       base, PreProcessInternal

(where Main.hsc includes asdf.h).

@ezyang ezyang added this to the ⊥ milestone Sep 6, 2016
@ezyang ezyang modified the milestones: Triaged, ⊥ Sep 8, 2016
@ezyang ezyang modified the milestones: 2.0, Triaged Oct 19, 2016
@ezyang
Copy link
Contributor Author

ezyang commented Oct 19, 2016

I accidentally fixed this on my way for another 2.0 blocker, so a patch is on the way.

ezyang added a commit to ezyang/cabal that referenced this issue Oct 19, 2016
@ezyang
Only consider dependencies in closure when computing -I on hsc2hs
e760cf7 
Previously, we unconditionally blasted in all packages, even
if our component didn't actually depend on them.
This fixes haskell#2971.

Also, added a test T2971a which is the opposite problem; previously
we didn't bring in include-dirs from internal libraries.  That
was fixed by the previous commit.

Signed-off-by: Edward Z. Yang <[email protected]>
ezyang added a commit to ezyang/cabal that referenced this issue Oct 19, 2016
@ezyang
Only consider dependencies in closure when computing -I on hsc2hs
83aecf4 
Previously, we unconditionally blasted in all packages, even
if our component didn't actually depend on them.
This fixes haskell#2971.

Also, added a test T2971a which is the opposite problem; previously
we didn't bring in include-dirs from internal libraries.  That
was fixed by the previous commit.

Signed-off-by: Edward Z. Yang <[email protected]>
@ezyang ezyang mentioned this issue Oct 19, 2016
Closed
ezyang added a commit to ezyang/cabal that referenced this issue Oct 19, 2016
@ezyang
Only consider dependencies in closure when computing -I on hsc2hs
150d156 
Previously, we unconditionally blasted in all packages, even
if our component didn't actually depend on them.
This fixes haskell#2971.

Also, added a test T2971a which is the opposite problem; previously
we didn't bring in include-dirs from internal libraries.  That
was fixed by the previous commit.

Signed-off-by: Edward Z. Yang <[email protected]>
@ezyang ezyang closed this as completed in b736896 Oct 20, 2016
@ezyang ezyang mentioned this issue Oct 25, 2016
Closed
ezyang added a commit to ezyang/cabal that referenced this issue Oct 25, 2016
@ezyang
Don't use installedPkgs for internal library library dirs.
3d9a9c5 
In 1.24, installedPkgs contained only references to the external package
database, not any internal libraries. In particular, when we built a
dynamically linked executable, installedPkgs did NOT have a reference to
the internal library; instead, depLibraryPaths has a special case
(hasInternalDeps) to add the final libdir to the RPATH.

In HEAD, after 0d15ede, we started adding internal libraries (with the
INPLACE registrations) to installedPkgs to fix haskell#2971.  But depLibraryPaths
was not updated, which means that the inplace registrations got picked
up and added to the RPATH, resulting in bad temporary directories
showing up in RPATHs.

In this commit, we just filter out internal entries from installedPkgs
and compute the library dirs for them from scratch (this code already
existed, so no loss!)

Fixes haskell#4025.

Signed-off-by: Edward Z. Yang <[email protected]>
@ezyang ezyang mentioned this issue Oct 25, 2016
Merged
ezyang added a commit to ezyang/cabal that referenced this issue Oct 25, 2016
@ezyang
Don't use installedPkgs for internal library library dirs.
a1107e2 
In 1.24, installedPkgs contained only references to the external package
database, not any internal libraries. In particular, when we built a
dynamically linked executable, installedPkgs did NOT have a reference to
the internal library; instead, depLibraryPaths has a special case
(hasInternalDeps) to add the final libdir to the RPATH.

In HEAD, after 0d15ede, we started adding internal libraries (with the
INPLACE registrations) to installedPkgs to fix haskell#2971.  But depLibraryPaths
was not updated, which means that the inplace registrations got picked
up and added to the RPATH, resulting in bad temporary directories
showing up in RPATHs.

In this commit, we just filter out internal entries from installedPkgs
and compute the library dirs for them from scratch (this code already
existed, so no loss!)

Fixes haskell#4025.

Signed-off-by: Edward Z. Yang <[email protected]>
erikd pushed a commit to erikd/cabal that referenced this issue Jan 28, 2017
@ezyang @erikd
Don't use installedPkgs for internal library library dirs.
513a10a 
In 1.24, installedPkgs contained only references to the external package
database, not any internal libraries. In particular, when we built a
dynamically linked executable, installedPkgs did NOT have a reference to
the internal library; instead, depLibraryPaths has a special case
(hasInternalDeps) to add the final libdir to the RPATH.

In HEAD, after 0d15ede, we started adding internal libraries (with the
INPLACE registrations) to installedPkgs to fix haskell#2971.  But depLibraryPaths
was not updated, which means that the inplace registrations got picked
up and added to the RPATH, resulting in bad temporary directories
showing up in RPATHs.

In this commit, we just filter out internal entries from installedPkgs
and compute the library dirs for them from scratch (this code already
existed, so no loss!)

Fixes haskell#4025.

Signed-off-by: Edward Z. Yang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Cabal: other type: bug
Projects
None yet
Milestone
2.0
1 participant
@ezyang