Skip to content

Add mpark variant to dependency as alternative of cxx17 variant - #4290

Merged
vigsterkr merged 2 commits into
shogun-toolbox:developfrom
vinx13:features/variant
May 22, 2018
Merged

Add mpark variant to dependency as alternative of cxx17 variant#4290
vigsterkr merged 2 commits into
shogun-toolbox:developfrom
vinx13:features/variant

Conversation

@vinx13

@vinx13 vinx13 commented May 18, 2018

Copy link
Copy Markdown
Member

No description provided.

@vinx13 vinx13 changed the title Add mpack variant to dependency as alternative of cxx17 variant Add mpark variant to dependency as alternative of cxx17 variant May 18, 2018
@vinx13
vinx13 force-pushed the features/variant branch from 288f64d to 3c7e1e8 Compare May 18, 2018 10:07
SOURCE_DIR ${VARIANT_SOURCE_DIR}
GIT_REPOSITORY https://github.com/mpark/variant.git
GIT_SHALLOW 1
GIT_TAG single-header

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this should be ${VARIANT_RELEASE_VERSION} no?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this is a branch for release, each version is a single directory

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yeah but they have tags as well, see https://github.com/mpark/variant/tree/v1.3.0

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

because unfortunately the branch HEAD will always contain the latest release, meaning you will always checkout the latest release which probably not going to be in future VARIANT_RELEASE_VERSION and it's a good idea in general to depend on a specific version of an external lib.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

install with provided cmakelist requires a higher version cmake than ours
https://github.com/mpark/variant/blob/29319715a1f0eb0980d380db8a2fda5af8d58feb/CMakeLists.txt#L8

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

mmmm i've just checked that branch, sorry i totally misunderstood the concept this... it's pretty weird to me having a branch totally different content than the base branch :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ignore my comments above

@vigsterkr

Copy link
Copy Markdown
Member

@vinx13 i'll ping you when you should rebase this branch over the latest develop so we can get an actual good picture of the CI status... sorry it is my fault :(

@vigsterkr

Copy link
Copy Markdown
Member

@vinx13 could you please rebase your branch over latest develop and push it so that this PR gets properly tested. thnx!

@vinx13
vinx13 force-pushed the features/variant branch from 02f8897 to ff69047 Compare May 18, 2018 14:14
@vigsterkr
vigsterkr merged commit 57903ff into shogun-toolbox:develop May 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants