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

cmake: Generate obj/build.h header #51

Merged
merged 1 commit into from
Nov 16, 2023
Merged

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented Nov 12, 2023

No description provided.

@@ -10,7 +10,21 @@ include_directories(${CMAKE_CURRENT_BINARY_DIR} ${CMAKE_CURRENT_SOURCE_DIR})

add_subdirectory(crypto)
add_subdirectory(univalue)

add_custom_target(generate_build_info
Copy link

Choose a reason for hiding this comment

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

Hmm, I don't think this will re-run on each make, so it won't pick up new git checkouts will it?

In other words: in master, switching git branches, or creating a new commit, will update build.h automatically on the next make. I don't think that's the case here?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hmm, I don't think this will re-run on each make, so it won't pick up new git checkouts will it?

In other words: in master, switching git branches, or creating a new commit, will update build.h automatically on the next make. I don't think that's the case here?

The target added by the add_custom_target command:

... is always considered out of date...

This ensures that the obj/build.h header is generated from scratch on every build attempt.

Copy link

Choose a reason for hiding this comment

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

Huh, ok, yep. I didn't get that at all from reading the code, but sure enough that's how it works :)

@theuni
Copy link

theuni commented Nov 15, 2023

Found this from a quick googling, seems to be the approach we're after: https://jonathanhamberg.com/post/cmake-embedding-git-hash/

@hebasto
Copy link
Owner Author

hebasto commented Nov 16, 2023

Rebased.

Copy link

@theuni theuni left a comment

Choose a reason for hiding this comment

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

ACK after adding a comment (if you agree)

add_subdirectory(util)
add_dependencies(bitcoin_util generate_build_info)
Copy link

Choose a reason for hiding this comment

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

It's unfortunate that this is less specific than what we have in master: depending on the entire library rather than making the relationship explicit:

libbitcoin_util_a-clientversion.$(OBJEXT): obj/build.h

But I suppose this will work just fine.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks! Reworked.

Copy link

Choose a reason for hiding this comment

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

Oh nice :)

@@ -10,7 +10,21 @@ include_directories(${CMAKE_CURRENT_BINARY_DIR} ${CMAKE_CURRENT_SOURCE_DIR})

add_subdirectory(crypto)
add_subdirectory(univalue)

add_custom_target(generate_build_info
BYPRODUCTS ${PROJECT_BINARY_DIR}/src/obj/build.h
Copy link

Choose a reason for hiding this comment

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

Note that the obj is catering to autotools, trying to keep the generated files away from the source ones. In a follow-up after CMake merge, we could drop the obj/ prefix. Mind adding a note?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks! Done.

@hebasto
Copy link
Owner Author

hebasto commented Nov 16, 2023

Addressed all @theuni's comments.

Copy link

@theuni theuni left a comment

Choose a reason for hiding this comment

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

ACK 5c2c0f3

@hebasto hebasto merged commit 0fdf4f1 into cmake-staging Nov 16, 2023
8 checks passed
hebasto added a commit that referenced this pull request Nov 26, 2023
2e2a7b5 fixup! ci: Test CMake edge cases (Hennadii Stepanov)
999527d fixup! ci: Test CMake edge cases (Hennadii Stepanov)
9591677 cmake: Add `GenerateBuildInfo.cmake` script (Hennadii Stepanov)

Pull request description:

  This PR replaces shell `share/genbuild.sh` script with CMake `GenerateBuildInfo.cmake` script, which works on all platforms, including Windows.

  Fixes build on Windows after #51.

  Please note, that in the master branch, MSVC builds lack build info entirely.

  Added a CI test to ensure equivalence of the script replacement.

ACKs for top commit:
  pablomartin4btc:
    tACK 2e2a7b5

Tree-SHA512: d0a4c1bbc3335a06f60fbc5752e1ab7191fcf69002f72e0109de9a5f3c9b735a0a0cfa17becadf453fe7cba521983101e6cb34b0c60a11b95c43e7658675f077
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.

2 participants