doc: Remove Visual Studio 2017 reference from readme #21811
Conversation
|
Concept ACK on bringing the docs in line with the facts. |
|
I was not able to build using VS 2019 last time I tried: https://bitcoin.stackexchange.com/questions/99241/anyone-tried-building-bitcoin-core-with-visual-studio-2019 Still Concept ACK because I don't expect things to work with VS 2017 and agree with @sipsorcery that It does not seem worth the effort to try and support VS2017. |
jarolrod
left a comment
jarolrod
left a comment
There was a problem hiding this comment.
Concept ACK
Maybe unrelated? But, what about these references to Visual Studio 2017?
bitcoin/src/leveldb/.appveyor.yml
Line 11 in 2b45cf0
Line 45 in 2b45cf0
And here Visual Studio 2016
bitcoin/build_msvc/bitcoin.sln
Line 3 in 2b45cf0
|
Approach ACK 39656f3b689c513aa3c509682e07dece85002128 I think you could squash the commits, and include that you are adding this warning in the commit message of 42cf3b3a65c20030b909c1dd6219aca7d5833299 |
|
I understand the preferred approach these days is to leave PR changes as individual commits. The GitHub merge feature has the option to squash commits and combine their messages. |
|
Our project doesn't use the merge button, but a specific merge script to preserve the git history and collect reviews on it. (Reviews are invalidated when the commit id changes, such as when squashing, even when done during a merge). |
|
Squashed. |
jarolrod
left a comment
jarolrod
left a comment
There was a problem hiding this comment.
ACK 51f17c4dbd46d25e2352e0a5cbfad4d438673a10
two minor nits, but I wouldn't want to hold this up over commas 🥃
This PR was motivated by a comment in GUI PR (257) regarding a suggested improvement not being supported by VS2017. When checking whether master can still be built with the VS2017 toolset ABI issues were encountered. Most likely due to the pre-compiled Qt binaries that are used. It does not seem worth the effort to try and support VS2017, which would most likely require additional Qt binaries, or lengthy instructions on how to build static Qt binaries on Windows (which is very error prone and tedious). Added advisory note about build not working with earlier Visual Studio versions. Fixed grammar.
|
ACK 0a33145 |
fanquake
changed the title
6 participants
This PR was motivated by a comment in GUI PR (257) regarding a suggested improvement not being supported by VS2017.
When checking whether master can still be built with the VS2017 toolset ABI issues were encountered. Most likely due to the pre-compiled Qt binaries that are used.
It does not seem worth the effort to try and support VS2017, which would most likely require additional Qt binaries, or lengthy instructions on how to build static Qt binaries on Windows (which is very error prone and tedious).