Skip to content

Conversation

will-lauer
Copy link
Contributor

A bunch of cmake changes to improve a bunch of things:

  • Add missing files
  • enable package building
  • enable cmake structure to support importing with find_package

@coveralls
Copy link

coveralls commented Oct 11, 2021

Pull Request Test Coverage Report for Build 1355799137

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+5.2%) to 97.93%

Totals Coverage Status
Change from base Build 1317592223: 5.2%
Covered Lines: 7427
Relevant Lines: 7584

💛 - Coveralls

@will-lauer will-lauer marked this pull request as ready for review October 25, 2021 19:03
Copy link
Contributor

@jmalkin jmalkin left a comment

Choose a reason for hiding this comment

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

A couple questions but nothing that's a blocker.

Installing all files into a single directory makes me think we need to revisit file naming for a few sketches. Mostly HLL (which is my fault).

set(datasketches_INSTALL_DIR ${INSTALL_DIR})
message("Source dir of datasketches = ${datasketches_INSTALL_DIR}")
target_include_directories(my_dependent_target
PRIVATE ${datasketches_INSTALL_DIR}/include/DataSketches)
Copy link
Contributor

Choose a reason for hiding this comment

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

PRIVATE seems to be spaced way over for some reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this spacing is because I use 4-space tabs in my editor. The file is a bit of a mix between tabs and spaces (and some of that is my fault), with tab seeming to be the default. It looks like git is showing the data using 8-space tabs, which makes it line up differently than in my editor.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer spaces for consistency, but this is not critical


```
find_package(DataSketches 3.2 REQUIRED)
target_link_library(my_dependent_target PUBLIC ${DATASKETCHES_LIB})
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we provide the set (of size 1, I think?) of exported variables somewhere?

I guess related, do we want to expose the individual sketches or just have people depend on the entire library? I think it'll only pull in relevant files since we're header-only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, a list of the environment variables is likely useful in the doc. In fact, it should probably be expanded to more than one, as both include directory, library, and target are useful.

@will-lauer
Copy link
Contributor Author

In response you @jmalkin's single directory comment, the files are named such that there are no collisons right now, but I suspect that restructuring the install image so that there is a toplevel datasketches directory that contains separate directories for each individual sketch type, plus a common directory, would be a better layout. Unfortunately, that has a fairly high impact, as it potentially changes the #include statements both in the code and in client code.

@AlexanderSaydakov AlexanderSaydakov merged commit 710ac0a into apache:master Oct 28, 2021
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.

4 participants