doc: Remove Visual Studio 2017 reference from readme #21811

sipsorcery commented at 9:56 PM on April 29, 2021: member

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).

hebasto commented at 10:09 PM on April 29, 2021: member

Concept ACK on bringing the docs in line with the facts.

ghost commented at 10:44 PM on April 29, 2021: none

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.

DrahtBot added the label Docs on Apr 29, 2021

in build_msvc/README.md:72 in 42cf3b3a65 outdated

  67 |  
  68 |  ```
  69 |  msbuild /m bitcoin.sln /p:Platform=x64 /p:Configuration=Release /t:build
  70 |  ```
  71 |  
  72 | -- Alternatively open the `build_msvc/bitcoin.sln` file in Visual Studio.

jarolrod commented at 6:12 PM on April 30, 2021:

If we don't have a note that < Visual Studio 2019 is not supported, won't this lead to people opening issues that they can't build with an earlier version?

sipsorcery commented at 7:31 PM on April 30, 2021:

I believe the leveldb appveyor file and readme are from the upstream repo. EIther way they are not used in the msvc CI for Bitcoin Core.

There is no Visual Studio 2016 application. Instead to make things super confusing Microsoft have used version 16.x for Visual Studio 2019, see https://en.wikipedia.org/wiki/Microsoft_Visual_Studio#History.

That's a good point about the build not working with earlier versions of Visual Studio. I'll adjust the PR to add a note to that effect.

jarolrod commented at 6:13 PM on April 30, 2021: member

Concept ACK

Maybe unrelated? But, what about these references to Visual Studio 2017?

https://github.com/bitcoin/bitcoin/blob/2b45cf0bcdb3d2c1de46899e30885c953b57b475/src/leveldb/.appveyor.yml#L11

https://github.com/bitcoin/bitcoin/blob/2b45cf0bcdb3d2c1de46899e30885c953b57b475/src/leveldb/README.md#L45

And here Visual Studio 2016

https://github.com/bitcoin/bitcoin/blob/2b45cf0bcdb3d2c1de46899e30885c953b57b475/build_msvc/bitcoin.sln#L3

jarolrod commented at 11:26 PM on April 30, 2021: member

Approach ACK 39656f3b689c513aa3c509682e07dece85002128

I think you could squash the commits, and include that you are adding this warning in the commit message of 42cf3b3a65c20030b909c1dd6219aca7d5833299

sipsorcery commented at 7:36 AM on May 1, 2021: member

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.

MarcoFalke commented at 7:39 AM on May 1, 2021: member

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).

sipsorcery force-pushed on May 1, 2021

sipsorcery commented at 7:46 AM on May 1, 2021: member

Squashed.

in build_msvc/README.md:66 in 51f17c4dbd outdated

  68 |  ```
  69 |  msbuild /m bitcoin.sln /p:Platform=x64 /p:Configuration=Release /t:build
  70 |  ```
  71 |  
  72 | -- Alternatively open the `build_msvc/bitcoin.sln` file in Visual Studio.
  73 | +- Alternatively open the `build_msvc/bitcoin.sln` file in Visual Studio 2019.

jarolrod commented at 4:06 PM on May 1, 2021:

while here, I guess we could fix the punctuation

- Alternatively, open the `build_msvc/bitcoin.sln` file in Visual Studio 2019.

in build_msvc/README.md:38 in 51f17c4dbd outdated

  34 | @@ -35,7 +35,7 @@ vcpkg integrate install
  35 |  
  36 |  Qt
  37 |  ---------------------
  38 | -In order to build the Bitcoin Core a static build of Qt is required. The runtime library version (e.g. v141, v142) and platform type (x86 or x64) must also match.
  39 | +In order to build the Bitcoin Core a static build of Qt is required. The runtime library version (e.g. v142) and platform type (x86 or x64) must also match.

jarolrod commented at 4:09 PM on May 1, 2021:

In order to build Bitcoin Core, a static build of Qt is required. The runtime library version (e.g. v142) and platform type (x86 or x64) must also match.

jarolrod commented at 4:10 PM on May 1, 2021: member

ACK 51f17c4dbd46d25e2352e0a5cbfad4d438673a10

two minor nits, but I wouldn't want to hold this up over commas 🥃

Remove Visual Studio 2017 reference from readme

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.

0a331456e4

sipsorcery force-pushed on May 1, 2021

jarolrod commented at 5:45 PM on May 1, 2021: member

ACK 0a331456e44ce2bdd3bfe62f7603a312905c6624

hebasto approved

hebasto commented at 5:47 PM on May 1, 2021: member

ACK 0a331456e44ce2bdd3bfe62f7603a312905c6624, I have reviewed the code and it looks OK, I agree it can be merged.

fanquake renamed this:
~~Remove Visual Studio 2017 reference from readme~~
doc: Remove Visual Studio 2017 reference from readme
on May 2, 2021

fanquake merged this on May 2, 2021

fanquake closed this on May 2, 2021

sidhujag referenced this in commit 797fdb7458 on May 2, 2021

gwillen referenced this in commit ab260dd10b on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022