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

Fix header and pkg-config file installation #426

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

turboencabulator
Copy link
Contributor

This reverts some undesirable changes introduced in the 3.9.0 release:

@fghoussen
Copy link
Collaborator

fghoussen commented Aug 14, 2023

Likely conflict with #425. Can you rebase?
Also recall that for compatibility reasons usr/include/arpack is prefered #394 (comment) #398 (comment)

Note: #425 killed arpackSolver.pc as when using pc files you need to "add up" dependencies #425 (comment) so to use arpackSolver you just need to add eigen like you'll need to add blas or lapack

The autotools build is configured to skip entire directories depending
on the configuration.  Each directory's Makefile is responsible for
installing what it needs, and moving the .pc files to the toplevel broke
that.

In the pkg-config files, leave ${includedir} unmodified from how
configure defines it.
@turboencabulator
Copy link
Contributor Author

@fghoussen Done. Looks like you've already fixed a couple of the same issues I found.

I did just notice that CMake is installing arpackicb.h even when the ICB option is not enabled, is that a bug?

@fghoussen
Copy link
Collaborator

I'll have a closer look when possible.
Can you move back pc files in pkg-config dir? This way all .cmake/.pc files (dealing with finding cmake-package or pkg-config-module) are at the same level in a dedicated directory with associated tests (sh)

@fghoussen Done. Looks like you've already fixed a couple of the same issues I found.

Yep ! :)

I did just notice that CMake is installing arpackicb.h even when the ICB option is not enabled, is that a bug?

AFAIR, arpackicb.his meant to define types for fortran subroutines

@fghoussen
Copy link
Collaborator

I did just notice that CMake is installing arpackicb.h even when the ICB option is not enabled, is that a bug?

I think arpackicb.h can be installed only when OCB=ON. afd8413 will tell...

@turboencabulator
Copy link
Contributor Author

Can you move back pc files in pkg-config dir? This way all .cmake/.pc files (dealing with finding cmake-package or pkg-config-module) are at the same level in a dedicated directory with associated tests (sh)

I looked into a couple different ways to do it and I wasn't happy with either. The makefiles would reference things outside of their directory, or we would add more conditional stuff at the toplevel that is easy to mess up. I think it's better to keep the .pc files along with the library code that each .pc file is supposed to describe.

What about moving your test scripts to a common test directory instead? The .pc files aren't specific to any particular build system (unlike cmake) so they don't need to live alongside the autotools test.

@fghoussen
Copy link
Collaborator

I have never been happy with that neither but I'd like at least to keep things symmetric.

The makefiles would reference things outside of their directory

That's why at some point I moved all files (*.pc, *.cmake, tst*.sh) at the root level: you know where things are

better to keep the .pc files along with the library code that each .pc file

There is 1 *.cmake file for both arpack and parpack, but, 2 *.pc file for each of them.
Do we want to split *.cmake in 2 to have one for arpack and another one for parpack?.... Is this worth?!...

@turboencabulator
Copy link
Contributor Author

There is 1 *.cmake file for both arpack and parpack, but, 2 *.pc file for each of them. Do we want to split *.cmake in 2 to have one for arpack and another one for parpack?.... Is this worth?!...

I don't know enough about cmake to have an opinion on this. For pkg-config the best practice seems to be having one .pc description file for each library/module.

@fghoussen
Copy link
Collaborator

IMO, the repo should be symmetric (as arpack is!). *.cmake can be split: no time for that...

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.

None yet

3 participants