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

Look for install includes in buildpref as well #4866

Merged
merged 3 commits into from
Nov 8, 2017

Conversation

angerman
Copy link
Collaborator

@angerman angerman commented Nov 7, 2017

When installing a lirbary, we should look for it's headers in it's 'build' preference first, then relative to the current path.

Please include the following checklist in your PR:

  • 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.

Please also shortly describe how you tested your change. Bonus points for added tests!

When installing a lirbary, we should look for it's headers in it's 'build' preference first, then relative to the current path.
@angerman
Copy link
Collaborator Author

angerman commented Nov 7, 2017

Alright, so ideally we'd look into the relative directories from the cabal file. This will work when the CWD is the folder containing the cabal file. However if it is not, and the generic package description in passed in, we have no handle on the location of the cabal file. Or do we?

However, Looking for the include headers in the build directory, prior to trying to locate them relative to the current path seems like a sensible change to me.

@angerman angerman force-pushed the feature/header-from-buildpref branch from 863cdba to a7db062 Compare November 7, 2017 08:43
Cabal/changelog Outdated
@@ -1,6 +1,8 @@
-*-change-log-*-

2.2.0.0 (current development version)
* `copyCompoment` and `installIncludeFiles` will look for include
headers in the build preference as well.
Copy link
Member

Choose a reason for hiding this comment

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

Should say what the default build preference dir is, i.e.:

... will look for include headers in the build preference dir ('dist/build/...' by default) as well.

@23Skidoo
Copy link
Member

23Skidoo commented Nov 7, 2017

When installing a lirbary, we should look for it's headers in it's 'build' preference first, then relative to the current path.

That's because custom setup scripts can put stuff there? OK, makes sense.

/cc @dcoutts

@hvr
Copy link
Member

hvr commented Nov 7, 2017

Can you explain the motivation and/or the problem this solving?

@angerman
Copy link
Collaborator Author

angerman commented Nov 7, 2017

scratch that.

@angerman
Copy link
Collaborator Author

angerman commented Nov 7, 2017

Can you explain the motivation and/or the problem this solving?

Alright, so you end up "generating" your headers. And they end up in the build directory, however your cabal file does addresses them relative.

@angerman
Copy link
Collaborator Author

angerman commented Nov 7, 2017

To clarify why I called this "build preference". This is due wo the variable naming of buildPref in the source.

@23Skidoo
Copy link
Member

23Skidoo commented Nov 7, 2017

@angerman To the users it's either dist/build/... or --builddir.

@hvr
Copy link
Member

hvr commented Nov 7, 2017

Oh I see; one thing though, for *-modules we have autogen-modules, in order to be explicit about what is autogenerated and which things cabal is supposed to warn us about.

I think we should have the corresponding facility also for installed headers, so we can have cabal warn at sdist time properly, when you specify a header that doesn't exist yet (because it will (not) be autogenerated)

@angerman
Copy link
Collaborator Author

angerman commented Nov 7, 2017

@23Skidoo I'll update the change log to reflect that better.

@angerman
Copy link
Collaborator Author

angerman commented Nov 7, 2017

To better understand where I'm coming from. I'm currently trying to roll ghc-cabal into hadrian. And all these issues crop up while doing so.

@23Skidoo 23Skidoo merged commit 3c7e33a into haskell:master Nov 8, 2017
@23Skidoo
Copy link
Member

23Skidoo commented Nov 8, 2017

Merged, thanks!

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.

3 participants