Skip to content

Conversation

@benjeffery
Copy link
Member

@benjeffery
Copy link
Member Author

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Dec 2, 2021

rebase

✅ Branch has been successfully rebased

@codecov
Copy link

codecov bot commented Dec 2, 2021

Codecov Report

Merging #1983 (8e66a8c) into main (9105f16) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1983   +/-   ##
=======================================
  Coverage   93.35%   93.35%           
=======================================
  Files          27       27           
  Lines       25521    25521           
  Branches     1111     1111           
=======================================
  Hits        23826    23826           
  Misses       1660     1660           
  Partials       35       35           
Flag Coverage Δ
c-tests 92.37% <ø> (ø)
lwt-tests 89.05% <ø> (ø)
python-c-tests 72.09% <ø> (ø)
python-tests 98.79% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9105f16...8e66a8c. Read the comment docs.

Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

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

Looks great!

Could we add a test like this one to tests/test_core.c please? See the kastore meson.build for how to define the MESON_VERSION.

@benjeffery
Copy link
Member Author

@jeromekelleher test added!

@benjeffery
Copy link
Member Author

@megifyio rebase

@jeromekelleher
Copy link
Member

@Mergifyio rebase

@jeromekelleher
Copy link
Member

(misspelled above @benjeffery )

@mergify
Copy link
Contributor

mergify bot commented Dec 3, 2021

rebase

✅ Branch has been successfully rebased

Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

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

LGTM @benjeffery - might be worth renaming a few things though just in case there's any confusion about which version of the version string comes first though!

c/meson.build Outdated
sources: ['tests/test_core.c'],
link_with: [tskit_lib, test_lib], c_args: extra_c_args, dependencies: kastore_dep)
link_with: [tskit_lib, test_lib], c_args: extra_c_args, dependencies: kastore_dep,
c_args: ['-DTSKIT_VERSION="@0@"'.format(meson.project_version())])
Copy link
Member

Choose a reason for hiding this comment

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

I used MESON_VERSION in kastore as a way of indicating "this is the version that meson thinks we're at", since it's not used at all otherwise. I guess there's not much chance of confusion when we're just passing it in to test_core.c and we're documenting it like this? How about MESON_PROJECT_VERSION as being absolutely explicit here?

}

static void
test_tskit_version(void)
Copy link
Member

Choose a reason for hiding this comment

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

We're testing the meson project version agrees with the defines (which are authoritative). Good to name the test accordingly

@benjeffery benjeffery force-pushed the auto_tarball branch 4 times, most recently from 40096b9 to de176ef Compare December 3, 2021 12:18
@benjeffery
Copy link
Member Author

Yeah, those are better names, fixed.

Copy link
Member

@jeromekelleher jeromekelleher left a comment

Choose a reason for hiding this comment

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

Looks good, one missing search/replace


sprintf(
version, "%d.%d.%d", TSK_VERSION_MAJOR, TSK_VERSION_MINOR, TSK_VERSION_PATCH);
/* the TSKIT_VERSION define is passed in by meson when compiling */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/* the TSKIT_VERSION define is passed in by meson when compiling */
/* the MESON_PROJECT_VERSION define is passed in by meson when compiling */

@mergify mergify bot merged commit 161b334 into tskit-dev:main Dec 3, 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.

Ship C tarball

2 participants