Skip to content

Add range-v3.#10737

Merged
chriseth merged 1 commit intodevelopfrom
range-v3
Jan 11, 2021
Merged

Add range-v3.#10737
chriseth merged 1 commit intodevelopfrom
range-v3

Conversation

@ekpyron
Copy link
Copy Markdown
Collaborator

@ekpyron ekpyron commented Jan 11, 2021

Refs #8860

@ekpyron ekpyron marked this pull request as ready for review January 11, 2021 17:26
@ekpyron ekpyron requested review from axic and chriseth January 11, 2021 17:30
# Fetch jsoncpp dependency
mkdir -p ./solc/deps/downloads/ 2>/dev/null || true
wget -O ./solc/deps/downloads/jsoncpp-1.9.3.tar.gz https://github.com/open-source-parsers/jsoncpp/archive/1.9.3.tar.gz
wget -O ./solc/deps/downloads/range-v3-0.11.0.tar.gz https://github.com/ericniebler/range-v3/archive/0.11.0.tar.gz
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you try this?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Not yet as an actual PPA build, no... but putting it there has it not-redownload, so it should work just as for jsoncpp.


add_library(solutil ${sources})
target_link_libraries(solutil PUBLIC jsoncpp Boost::boost Boost::filesystem Boost::system)
target_link_libraries(solutil PUBLIC jsoncpp Boost::boost Boost::filesystem Boost::system range-v3)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's not header-only?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It is - this pulls in the include directory only - it's an "INTERFACE" library for cmake - that's what they call header-only libraries.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

(We could have skipped it here, because the jsoncpp will also already add build/deps/include as include directory - but it's cleaner if we do it for range-v3 as well - but this does nothing else than exactly that plus making cmake aware of the dependency, s.t. it actually executes the ExternalProject_Add stuff.)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah right!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i would like get merged

CMAKE_ARGS -DCMAKE_INSTALL_PREFIX=<INSTALL_DIR>
-DCMAKE_CXX_COMPILER=${CMAKE_CXX_COMPILER}
-DBUILD_TESTING=OFF
-DRANGES_CXX_STD=17
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it possible to take this from our config? If not, can you add a comment to where we set it so that we don't forget to update this?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Changed it. Actually we probably wouldn't even need it, since this actually doesn't build anything, since we disable tests and examples, but still - it now uses ${CMAKE_CXX_STANDARD} which is set by us.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah nice!

@ekpyron
Copy link
Copy Markdown
Collaborator Author

ekpyron commented Jan 11, 2021

That was easy :-).

@chriseth chriseth merged commit 67d21a8 into develop Jan 11, 2021
@chriseth chriseth deleted the range-v3 branch January 11, 2021 18:24
@leonardoalt
Copy link
Copy Markdown

Oh nice!

@axic
Copy link
Copy Markdown
Contributor

axic commented Jan 11, 2021

@ekpyron well, you used external project, so of course it was going to be easy :) (There's precedent for that in the build system.)

@ekpyron
Copy link
Copy Markdown
Collaborator Author

ekpyron commented Jan 11, 2021

Yeah, well, thanks for #8860 (comment) ;-).

@ekpyron
Copy link
Copy Markdown
Collaborator Author

ekpyron commented Jan 12, 2021

I mean: I still think it's evil and mean, if make starts downloading stuff :-D. But that applies to any use of ExternalProject_add anyways, so since we already use it, adding a second instance doesn't exactly make it much worse - and once I ultimately convinced all of you of that, we can just change it for all of them at once :-).

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.

5 participants