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 tagtbl.C placement in build dir #2670

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

dmnks
Copy link
Contributor

@dmnks dmnks commented Sep 18, 2023

The COMMAND for tagtbl.C generation dumps the file in the top-level build directory whereas the OUTPUT is specified in the current (lib/) directory. This leads to unnecessary relinking of all the libraries on each make invocation because OUTPUT is always missing.

Fix this by outputting the file in the current directory where OUTPUT can find it.

No functional change, just makes rebuilds quicker.

@dmnks dmnks added the build Build-system related label Sep 18, 2023
The COMMAND for tagtbl.C generation dumps the file in the top-level
build directory whereas the OUTPUT is specified in the current (lib/)
directory.  This leads to unnecessary relinking of all the libraries on
each make invocation because OUTPUT is always missing.

Fix this by outputting the file in the current directory where OUTPUT
can find it.

No functional change, just makes rebuilds quicker.
@ffesti ffesti merged commit a25d881 into rpm-software-management:master Sep 19, 2023
@pmatilai
Copy link
Member

Ooh, good spotting! I remember there was some "reason" for doing it the way it was, but this would've been added during the first weeks of stumbling in the dark with cmake. So it probably was due to some other mistake, misunderstanding or such.

@dmnks
Copy link
Contributor Author

dmnks commented Sep 25, 2023

Heh, sure 😄

Actually, the output location was only changed in the recent commit 1c98b67 so it was more like just a forgotten entry in target_include_directories(librpm PRIVATE ...) where it didn't include the current build dir. So this commit was basically just a fixup adding that 😄

@pmatilai
Copy link
Member

Oh. So I'm misremembering something else related to tagtbl.C from the early days then. I wonder wtf I changed it in 1c98b67 for, doesn't seem to make any sense at all 😳

@dmnks
Copy link
Contributor Author

dmnks commented Sep 25, 2023

We actually include the top-level build directory in the top-level CMakeLists file, it even says # generated sources in a comment. So that could've been the impulse for you to output it there so that you didn't need to add the current build dir into the new include list in lib/CMakeLists.txt, I suppose.

At first, I thought the fix would be just to use OUTPUT ${CMAKE_BINARY_DIR}/tagtbl.C, however that turned out wrong because we link compile librpm against from that file (further up) and since it doesn't exist in the source dir, cmake tries to find it in the binary dir, and failing that, tries to find a target that builds it - which is this one. So putting a prefix such as ${CMAKE_BINARY_DIR} to it would cause cmake to not find it, and fail 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Build-system related REGRESSION
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants