-
Notifications
You must be signed in to change notification settings - Fork 11
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
render the local header systemwise #235
base: master
Are you sure you want to change the base?
render the local header systemwise #235
Conversation
Description: upstream: systemwise local header Make the local header `local.h' systemwise. Origin: vendor, Debian Author: Jerome Benoit <[email protected]> Last-Update: 2022-02-27
Codecov Report
@@ Coverage Diff @@
## master #235 +/- ##
=======================================
Coverage 92.00% 92.00%
=======================================
Files 105 105
Lines 2075 2075
=======================================
Hits 1909 1909
Misses 166 166
Continue to review full report at Codecov.
|
Thanks for the PR @jgmbenoit.
Where would such headers go typically? I only know about |
On Debian multi-arch boxes (e.g., i386 (32bits) + amd64 (64bits)) the hierarchy |
I see. Thanks for the explanation.
Wouldn't it always be installed into an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. It would be great if we could add some comments (as suggested) on the changes since they are not obvious (at least not to me). Also, I think you need to add the search path in https://github.com/flatsurf/e-antic/blob/master/doc/manual/Makefile.am#L6 for the docbuild to work.
Finally, could you add an entry in doc/news? See instructions in https://raw.githubusercontent.com/flatsurf/e-antic/master/.github/pull_request_template.md
@@ -13,7 +13,7 @@ | |||
#ifndef E_ANTIC_FMPQ_POLY_EXTRA_H | |||
#define E_ANTIC_FMPQ_POLY_EXTRA_H | |||
|
|||
#include "local.h" | |||
#include <e-antic/local.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#include <e-antic/local.h> | |
// Include architecture specific settings from e-antic/local.h and not just local.h since the header might be installed to separate architecture specific location on the search path. | |
#include <e-antic/local.h> |
Can we also add a disclaimer like that everywhere? Otherwise, we might just revert this in a few years when we have forgot why this was necessary I am afraid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idem
Indeed. Having said that, a "ceinture et bretelles" ("belt and braces") approach cannot hurt here. |
is this patch applied in version 1.2.0 ? |
No. I think there were still some open discussions here that we were waiting for. |
This patch renders the local header
local.h
systemwise.Note that this header should be actually installed in an architecture dependent include hierarchy.