More tweaks after standard updates #31

neatudarius · 2024-09-25T15:03:27Z

Missing changed from #14

Updates:

Apply CMAKE.CONFIG

neatudarius · 2024-09-25T15:09:53Z

CMakeLists.txt

 add_subdirectory(examples)
+
+write_basic_package_version_file(


Inspect current behavior from https://github.com/beman-project/execution26/blob/main/CMakeLists.txt and https://github.com/beman-project/optional26/blob/main/CMakeLists.txt.

I tried to extract only the minimum required code to do the install.

Expected tree:

$ cmake --install build --prefix /opt/beman.exemplar -- Install configuration: "" -- Installing: /opt/beman.exemplar/lib/libbeman.exemplar.a -- Installing: /opt/beman.exemplar/include -- Installing: /opt/beman.exemplar/include/beman -- Installing: /opt/beman.exemplar/include/beman/exemplar -- Installing: /opt/beman.exemplar/include/beman/exemplar/identity.hpp -- Installing: /opt/beman.exemplar/lib/cmake/beman.exemplarConfig.cmake -- Installing: /opt/beman.exemplar/lib/cmake/beman.exemplarConfigVersion.cmake $ tree /opt/beman.exemplar /opt/beman.exemplar ├── include │ └── beman │ └── exemplar │ └── identity.hpp └── lib ├── cmake │ ├── beman.exemplarConfig.cmake │ └── beman.exemplarConfigVersion.cmake └── libbeman.exemplar.a 6 directories, 4 files

I would like your feedback and decide a standard pattern, which will be applied initially to beman.examplar, and eventually mirror it into other 2 repos.

Open questions:

Are we OK to install only the next artifacts?

$export_path/include/beman/$short_name/* (all headers, in this repo - 1 file)

$export_path/lib/libbeman.short_name.a (1 file)

$export_path/lib/cmake/beman.short_name.Config[Version].cmake (2 files)
What we should NOT export:

example binaries (we do that for optional26 , I'll fix it)

test binaries

scripts

docs

@camio proposed to add a subdirectory for cmake configs - a.k.a. /opt/beman.exemplar/lib/cmake/beman.exemplarConfig.cmake -> /opt/beman.exemplar/lib/cmake/beman.exemplar/beman.exemplarConfig.cmake. Do people agree with that?

@steve-downey , do we need these lines https://github.com/beman-project/optional26/blob/main/CMakeLists.txt#L36-L39?

Waiting for your input here. If any decision will be made here, we can put it into Beman standard.

CC: @camio @steve-downey @dietmarkuehl @bretbrownjr

[W]e should NOT export...docs

I'm okay with this for now. Eventually, we should export docs at installation time to the docs/beman.short_name/ directory, but we need to get our docs story figured out first.

@camio proposed to add a subdirectory for cmake configs

Yes, this is the convention for the ArchLinux distribution and probably others as well. See openjpeg2's contents for an example. Config Mode's Search Procedure includes a table indicating how *Config.cmake files are found.

Eventually we should just have a function like install_beman_library that wraps up all of the above. Packaging for Arch, packaging for vcpkg, packaging for Conan, etc., are all different, and it would be a mistake to hardcode logic for each library for each environment. That's an MxN complexity algorithm. We need an abstraction that gives us an M+N task.

bretbrownjr

I have a few suggestions here and there. The most important changes I'd like to request are:

I suspect we're missing the Targets.cmake file from the installation logic, given the tree command doesn't note it
I suspect we're missing some sort of test to confirm the packaging works. I guess that could be another PR, but it would be nice to at least manually test that installing into a staging dir, manipulating CMAKE_PREFIX_PATH, and then calling find_package(exemplar REQUIRED) in an example works well. For what it's worth, Conan ships with a test that does this.
We'll want to do exact version matching or just ignore versions altogether (both work for me). Trying to support semantic versioning for locating a built binary is not a recipe for success. That's only really appropriate for intentionally ABI stable projects like libc++ or various C libraries. Certainly not Beman libraries, which are disclaiming stability, at least until they reach the "stable" status.

Minor general note: At some point in recent refactoring, we also lost the support for install COMPONENTS. We should add that back in, but it can be another PR.

FYI @steve-downey. I'm guessing we aren't using install components in optional26 either.

bretbrownjr · 2024-10-01T14:59:59Z

CMakeLists.txt

 add_subdirectory(examples)
+
+write_basic_package_version_file(


Eventually we should just have a function like install_beman_library that wraps up all of the above. Packaging for Arch, packaging for vcpkg, packaging for Conan, etc., are all different, and it would be a mistake to hardcode logic for each library for each environment. That's an MxN complexity algorithm. We need an abstraction that gives us an M+N task.

bretbrownjr · 2024-10-01T18:13:03Z

CMakeLists.txt

 include(CTest)
 include(FetchContent)
+include(GNUInstallDirs)
+
+set(TARGETS_EXPORT_NAME ${CMAKE_PROJECT_NAME}Targets)


I'd rather just inline this below instead of making a variable that's used only once.

bretbrownjr · 2024-10-01T18:14:57Z

CMakeLists.txt

 add_subdirectory(examples)
+
+write_basic_package_version_file(
+  ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}ConfigVersion.cmake


Suggested change

${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}ConfigVersion.cmake

${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}-config-version.cmake

Just because lower.caseThenCamelCase.cmake is not how anyone would choose to name a file. See upstream docs on support for kebab-case filenames.

bretbrownjr · 2024-10-01T18:15:30Z

CMakeLists.txt

+
+configure_package_config_file(
+  "cmake/Config.cmake.in"
+  ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}Config.cmake


Suggested change

${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}Config.cmake

${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}-config.cmake

See the comment about *ConfigVersion.cmake

bretbrownjr · 2024-10-01T18:17:18Z

CMakeLists.txt

 add_subdirectory(examples)
+


Shouldn't all the library metadata (i.e., exemplar-config.cmake, etc.) go in the src/beman/exemplar subdir?

bretbrownjr · 2024-10-01T18:21:56Z

CMakeLists.txt

+write_basic_package_version_file(
+  ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}ConfigVersion.cmake
+  VERSION ${PROJECT_VERSION}
+  COMPATIBILITY AnyNewerVersion)


Suggested change

COMPATIBILITY AnyNewerVersion)

COMPATIBILITY ExactVersion)

Unless we're doing something specific to guarantee some ABI compatibility across versions of a Beman project, we should be conservative about version matching. On the contrary, I fully expect API and ABI breaks across releases of Beman projects since they're by definition experimental and prone to design changes.

We could maybe revisit this choice on a case-by-case basis for projects that a "stable" status. But unless they're spelling their implementations with ABI tags and such, I wouldn't advise people bother.

bretbrownjr · 2024-10-01T18:22:16Z

CMakeLists.txt

+  INSTALL_DESTINATION ${INSTALL_CONFIGDIR})
+
+install(
+  FILES ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}Config.cmake


Suggested change

FILES ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}Config.cmake

FILES ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}-config.cmake

bretbrownjr · 2024-10-01T18:22:29Z

CMakeLists.txt

+
+install(
+  FILES ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}Config.cmake
+        ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}ConfigVersion.cmake


Suggested change

${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}ConfigVersion.cmake

${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}-config-version.cmake

bretbrownjr · 2024-10-01T18:27:47Z

README.md

+    │   ├── beman.exemplarConfig.cmake
+    │   └── beman.exemplarConfigVersion.cmake


Suggested change

│   ├── beman.exemplarConfig.cmake

│   └── beman.exemplarConfigVersion.cmake

│   ├── beman.exemplar-config.cmake

│   └── beman.exemplar-config-version.cmake

Also, what happened to the file referenced by this bit in the config file?
include("${CMAKE_CURRENT_LIST_DIR}/@[email protected]")

wusatosi · 2024-11-14T22:24:38Z

@neatudarius Mind if I contribute directly to this PR?

neatudarius · 2024-12-01T23:44:43Z

@neatudarius Mind if I contribute directly to this PR?

Sure, please contribute

wusatosi · 2024-12-03T17:30:00Z

@neatudarius Mind if I contribute directly to this PR?

Sure, please contribute

I am in my finals week, I no longer have extra capacity to work on this.

camio · 2024-12-04T19:02:57Z

There's was a review of this PR on October 1st, but no updates since. Should we close this PR and re-open it when someone wants to drive it forward?

neatudarius · 2024-12-04T20:01:32Z

There's was a review of this PR on October 1st, but no updates since. Should we close this PR and re-open it when someone wants to drive it forward?

I will finish it this week. Sorry!

neatudarius added 2 commits September 25, 2024 15:56

Root README: typo in links

93f1a46

Remove extra whitespace

1fc7388

neatudarius force-pushed the more-tweaks-after-standard-updates branch 2 times, most recently from c04aadb to 06ba4e1 Compare September 25, 2024 15:05

neatudarius commented Sep 25, 2024

View reviewed changes

neatudarius marked this pull request as ready for review September 25, 2024 15:19

neatudarius requested review from bretbrownjr, camio, dietmarkuehl and steve-downey as code owners September 25, 2024 15:19

Apply Beman standard: CMAKE.CONFIG

9446152

neatudarius force-pushed the more-tweaks-after-standard-updates branch from 06ba4e1 to 9446152 Compare September 25, 2024 15:26

bretbrownjr requested changes Oct 1, 2024

View reviewed changes

wusatosi mentioned this pull request Nov 25, 2024

Feature/support macos builds #66

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More tweaks after standard updates #31

More tweaks after standard updates #31

neatudarius commented Sep 25, 2024 •

edited

Loading

neatudarius Sep 25, 2024 •

edited

Loading

camio Sep 25, 2024

bretbrownjr Oct 1, 2024

bretbrownjr left a comment

bretbrownjr Oct 1, 2024

bretbrownjr Oct 1, 2024

bretbrownjr Oct 1, 2024

bretbrownjr Oct 1, 2024

bretbrownjr Oct 1, 2024

bretbrownjr Oct 1, 2024

bretbrownjr Oct 1, 2024

bretbrownjr Oct 1, 2024

bretbrownjr Oct 1, 2024

wusatosi commented Nov 14, 2024

neatudarius commented Dec 1, 2024

wusatosi commented Dec 3, 2024

camio commented Dec 4, 2024

neatudarius commented Dec 4, 2024

	${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}ConfigVersion.cmake
	${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}-config-version.cmake

	FILES ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}Config.cmake
	FILES ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_PROJECT_NAME}-config.cmake

		│ ├── beman.exemplarConfig.cmake
		│ └── beman.exemplarConfigVersion.cmake

		add_subdirectory(examples)

		write_basic_package_version_file(

		add_subdirectory(examples)

		write_basic_package_version_file(

More tweaks after standard updates #31

Are you sure you want to change the base?

More tweaks after standard updates #31

Conversation

neatudarius commented Sep 25, 2024 • edited Loading

neatudarius Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bretbrownjr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wusatosi commented Nov 14, 2024

neatudarius commented Dec 1, 2024

wusatosi commented Dec 3, 2024

camio commented Dec 4, 2024

neatudarius commented Dec 4, 2024

neatudarius commented Sep 25, 2024 •

edited

Loading

neatudarius Sep 25, 2024 •

edited

Loading