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

Add fmtlib dependency #1079

Merged
merged 1 commit into from Sep 7, 2023
Merged

Conversation

feltech
Copy link
Member

@feltech feltech commented Sep 5, 2023

Description

Closes #1070.

fmtlib allows for easy, safe string substitution in C++, adding advanced formatting functionality (e.g. rounding), and reducing bloat.

So add as a new private build-only header-only dependency.

Using header-only increases compile times, but subjectively it's not noticeable, and has the benefit of avoiding a possible public library dependency (if OpenAssetIO is built as a static library).

Tweak a single existing, tested, log string to make use of libfmt, to ensure basic usage is validated.

Added to openassetio-build Docker image: https://github.com/OpenAssetIO/OpenAssetIO/pkgs/container/openassetio-build

  • I have updated the release notes.
  • [ ] I have updated all relevant user documentation.

Reviewer Notes

Test Instructions

@feltech feltech self-assigned this Sep 5, 2023
@feltech feltech requested a review from a team as a code owner September 5, 2023 13:11
@feltech feltech force-pushed the work/1070-addFmtLib branch 2 times, most recently from a39eb06 to 0162836 Compare September 5, 2023 13:19
@foundrytom
Copy link
Collaborator

I know we removed CONDING_STANDARDS as there is only one way to use the header version - but its it worth adding a more general note that "we use libfmt for string building wherever needed, do not use stringstream etc..."

@feltech
Copy link
Member Author

feltech commented Sep 5, 2023

Added an emoji to the release notes to signal a build dependency, see what you think (open to ideas on alternative emojis).

I know we removed CONDING_STANDARDS as there is only one way to use the header version - but its it worth adding a more general note that "we use libfmt for string building wherever needed, do not use stringstream etc..."

Added (force-pushed).

foundrytom
foundrytom previously approved these changes Sep 5, 2023
@feltech
Copy link
Member Author

feltech commented Sep 5, 2023

Fixup pushed to reduce fmtlib version to 9.1.0, which doesn't export errant symbols. Also included the Makefile change for the Docker container.

@foundrytom
Copy link
Collaborator

Fixup pushed to reduce fmtlib version to 9.1.0, which doesn't export errant symbols. Also included the Makefile change for the Docker container.

Thanks for posting upstream does this comment imply we may have issues on Windows with the older version?

@foundrytom foundrytom dismissed their stale review September 6, 2023 08:08

Potential symbol visibility issues

@feltech
Copy link
Member Author

feltech commented Sep 6, 2023

Fixup pushed to reduce fmtlib version to 9.1.0, which doesn't export errant symbols. Also included the Makefile change for the Docker container.

Thanks for posting upstream does this comment imply we may have issues on Windows with the older version?

As discussed offline - posted a follow-up question upstream to make sure I understand, but pretty certain we don't have a problem, especially in header-only mode.

doc/contributing/CODING_STANDARDS.md Show resolved Hide resolved
resources/build/Makefile Show resolved Hide resolved
Copy link
Collaborator

@foundrytom foundrytom left a comment

Choose a reason for hiding this comment

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

Assuming were happy with the symbol situation, then LGTM 👍 🙏

Closes OpenAssetIO#1070. fmtlib allows for easy, safe string substitution in C++,
adding advanced formatting functionality (e.g. rounding), and reducing
bloat.

So add as a new private build-only header-only dependency.

Using header-only increases compile times, but subjectively it's not
noticeable, and has the benefit of avoiding a possible public library
dependency (if OpenAssetIO is built as a static library).

Tweak a single existing, tested, log string to make use of libfmt, to
ensure basic usage is validated.

Signed-off-by: David Feltell <david.feltell@foundry.com>
@feltech feltech merged commit 8dba8ae into OpenAssetIO:main Sep 7, 2023
39 checks passed
@feltech feltech deleted the work/1070-addFmtLib branch September 7, 2023 13:05
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.

Add fmtlib dependency
3 participants