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

Makefile: Install the templated pkg-config file. #160

Merged
merged 1 commit into from
Apr 4, 2023

Conversation

Apteryks
Copy link
Contributor

Otherwise, the 'prefix' variable would be left undefined:

$ pkg-config --cflags mpc
Variable 'prefix' not defined in '/gnu/store/...-profile/lib/pkgconfig/mpc.pc'

$ cat /gnu/store/...-profile/lib/pkgconfig/mpc.pc
libdir=${prefix}/lib
includedir=${prefix}/include
[...]

  • Makefile (install): Install the templated pkg-config from the build directory.
    (all): Add the libs and $(DIST)/$(PROJ).pc targets.

Otherwise, the 'prefix' variable would be left undefined:

   $ pkg-config --cflags mpc
   Variable 'prefix' not defined in '/gnu/store/...-profile/lib/pkgconfig/mpc.pc'

   $ cat /gnu/store/...-profile/lib/pkgconfig/mpc.pc
   libdir=${prefix}/lib
   includedir=${prefix}/include
   [...]

* Makefile (install): Install the templated pkg-config from the build
directory.
(all): Add the libs and $(DIST)/$(PROJ).pc targets.
@HalosGhost
Copy link
Collaborator

utACK. Seems pretty clear that the install target didn't get updated properly to handle the templated .pc file.

Copy link
Collaborator

@HalosGhost HalosGhost 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! This patch ensures the installed .pc is the generated file, not the template.

It also ensures the libraries and .pc get built by-default which seems reasonable, if tangential.

@Apteryks
Copy link
Contributor Author

Apteryks commented Apr 3, 2023

Hello! About the tangential part: since the install target depends on 'all' and we want to install the generated .pc file, it seemed like easiest to ensure that by registering it in the 'all' target.

@HalosGhost
Copy link
Collaborator

HalosGhost commented Apr 3, 2023

Quite fair; I think the only thing that threw me was the addition of libs when those are guaranteed to be built by all anyway due to the dependencies from EXAMPLESEXE.

As a point-of-reference: it probably would have felt more clear what the goal was if those two dependencies had been added to install rather than changing all. But, it's also completely fair that all should really build all the things that can be built. So, my review doesn't change. 👍 from me.

@orangeduck orangeduck merged commit 0d75f36 into orangeduck:master Apr 4, 2023
@orangeduck
Copy link
Owner

Thanks all!

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