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

[DOCS] Add BSD build instructions #5955

Merged
merged 1 commit into from
Feb 19, 2019
Merged

[DOCS] Add BSD build instructions #5955

merged 1 commit into from
Feb 19, 2019

Conversation

ChrisChinchilla
Copy link
Contributor

@ChrisChinchilla ChrisChinchilla commented Feb 7, 2019

Description

Closes #5728 by adding BSD build instructions as mentioned by @flipchan I haven't been able to successfully test these running in an emulator on my Mac, but the original poster said they work. So either we test them elsewhere or add a disclaimer saying that they are untested?

Checklist

  • Code compiles correctly
  • All tests are passing
  • New tests have been created which fail without the change (if possible)
  • README / documentation was extended, if necessary
  • Changelog entry (if change is visible to the user)
  • Used meaningful commit messages

+-----------------------------------+-------------------------------------------------------+
| `Boost`_ | C++ libraries. |
+-----------------------------------+-------------------------------------------------------+
| `z3`_ | Theorem prover. |
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't say this is a prereq.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@axic BSD doesn't come with any of these packages, and you need them to build Solidity, so that's a requirement isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's an optional dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry @axic and @chriseth from the line comment I can't tell which dependency you're referring to. Does this mean that it's also optional for Linux, macOS etc too?

Copy link
Member

Choose a reason for hiding this comment

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

z3 is optional on all platforms. It is not listed on the others, shouldn't list it for BSD. Unless the install_deps script is broken and requires it, of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@axic Ah ha, OK, that was just taken from the original poster. And what do you think about adding the "untested" disclaimer?

@codecov
Copy link

codecov bot commented Feb 7, 2019

Codecov Report

Merging #5955 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #5955   +/-   ##
========================================
  Coverage    88.32%   88.32%           
========================================
  Files          361      361           
  Lines        34788    34788           
  Branches      4121     4121           
========================================
  Hits         30727    30727           
  Misses        2684     2684           
  Partials      1377     1377
Flag Coverage Δ
#all 88.32% <ø> (ø) ⬆️
#syntax 27.83% <ø> (ø) ⬆️

erak
erak previously approved these changes Feb 11, 2019
@@ -228,6 +228,28 @@ in Visual Studio 2017 Build Tools or Visual Studio 2017:
.. _Visual Studio 2017: https://www.visualstudio.com/vs/
.. _Visual Studio 2017 Build Tools: https://www.visualstudio.com/downloads/#build-tools-for-visual-studio-2017

Prerequisites - OpenBSD
Copy link
Contributor

Choose a reason for hiding this comment

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

The instructions here are extremely generic and do not look OpenBSD-specific. Perhaps rename this to "other" or something like that? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree @chriseth Our dependency scripts don't work on OpenBSD, so you do need these. I guess BSD is the most likely "other OS", but how about we use the language from above, something like "Other Unix-style OSs (e.g. BSD)", something like that? Just so the BSD part is searchable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm actually not sure we should encourage the use of the dependency script. Installing packages should not be taken lightly. Perhaps we should start the section about which dependencies are required and which are recommended in general and then go into detail about how exactly you do it on the specific systems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chriseth Sounds like a good idea, but do you think it gets out of the scope of this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, sounds like in scope for me, unless we have more detailed instructions for bsd

Copy link
Contributor

@chriseth chriseth left a comment

Choose a reason for hiding this comment

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

Should be moved to a generic section.

@ChrisChinchilla
Copy link
Contributor Author

try this @chriseth the PR really becomes less about BSD, but I think the changes tidy things up a bit. I tried to make using the dependency script a little less like a step you have to take, and more of a step you can take.

+-----------------------------------+-------------------------------------------------------+
| `Git`_ | Command-line tool for retrieving source code. |
+-----------------------------------+-------------------------------------------------------+
| `z3`_ (Optional) | For use with SMT checker. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Should also add cvc4 (Optional) - For use with SMT checker - http://cvc4.cs.stanford.edu/web/

+-----------------------------------+-------------------------------------------------------+
| `Git`_ | Command-line tool for retrieving source code. |
+-----------------------------------+-------------------------------------------------------+
| `z3`_ (Optional) | For use with SMT checker. |
Copy link
Contributor

Choose a reason for hiding this comment

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

z3: version 4.6.0 or newer

@ChrisChinchilla
Copy link
Contributor Author

Added extra details as mentioned @chriseth

Copy link
Contributor

@chriseth chriseth left a comment

Choose a reason for hiding this comment

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

Great! Please squash!

Add BSD instructions

Remove z3 dependency from OpenBSD

Add disclaimer

Generalise build instructions

Update docs/installing-solidity.rst

Co-Authored-By: ChrisChinchilla <[email protected]>

Add extra version details
@ChrisChinchilla
Copy link
Contributor Author

@chriseth Done.

@chriseth chriseth merged commit f74a139 into develop Feb 19, 2019
@axic axic deleted the docs-openbsd branch February 19, 2019 23:06
@flipchan
Copy link

Nice!

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.

6 participants