Skip to content

doc: Replace a link to Qt precompiled binaries with compile instructions #22890

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.

Sign up for GitHub

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

Jump to bottom
Merged
fanquake merged 3 commits into from
Oct 7, 2021
Merged

doc: Replace a link to Qt precompiled binaries with compile instructions #22890

fanquake merged 3 commits into from
Oct 7, 2021

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Sep 5, 2021

This PR replaces a link to Qt precompiled binaries with compile instructions that allow users to self-compile static Qt package which is required for building Bitcoin Core with Visual Studio.

@hebasto
Copy link
Member Author

hebasto commented Sep 5, 2021

cc @sipsorcery @Bosch-0 @jarolrod

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 5, 2021 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@sipsorcery
Copy link
Contributor

sipsorcery commented Sep 5, 2021

@hebasto concept ACK on the doc changes.

Changing the Qt path to C:\Qt_static has the potential to make it trickier to switch between Qt versions, however, I suspect that may be a problem specific to me.

@DrahtBot DrahtBot mentioned this pull request Sep 5, 2021
Merged
@hebasto hebasto force-pushed the 210904-msvc branch 2 times, most recently from b44c3e3 to ecb823e Compare September 5, 2021 11:18
@hebasto
Copy link
Member Author

hebasto commented Sep 5, 2021 •
edited
Loading

Updated 7b8b6c8 -> ecb823e (pr22890.01 -> pr22890.02, diff):

Changing the Qt path to C:\Qt_static has the potential to make it trickier to switch between Qt versions, however, I suspect that may be a problem specific to me.

Now a non-default path to static Qt could specified in the QTBASEDIR environment variable.

@hebasto
Copy link
Member Author

hebasto commented Sep 5, 2021

Updated ecb823e -> 42df8d5 (pr22890.02 -> pr22890.03, diff):

@hebasto
Copy link
Member Author

hebasto commented Sep 5, 2021 •
edited
Loading

Updated 42df8d5 -> 8e9410f (pr22890.03 -> pr22890.04, diff):

  • dropped -ltcg configure option because link-time code generation significantly (at times) increases the required disc space, and it should not be suggested to users by default.

jarolrod
jarolrod reviewed Sep 5, 2021
View reviewed changes
Copy link
Contributor

@jarolrod jarolrod left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Some notes on wording, I will test for correctness of instructions later.

Prepares hazmat suit to enter the windows environment

@hebasto
Copy link
Member Author

hebasto commented Sep 5, 2021

Updated 8e9410f -> c948757 (pr22890.04 -> pr22890.05, diff):

jarolrod
jarolrod reviewed Sep 5, 2021
View reviewed changes
@fanquake
Copy link
Member

fanquake commented Sep 6, 2021

There's no point adding the patching instructions in 9a434e8, just to drop them in a later commit (c948757).

@hebasto
Copy link
Member Author

hebasto commented Sep 6, 2021

Updated c948757 -> 0f56ce3 (pr22890.05 -> pr22890.06, diff):

@hebasto hebasto mentioned this pull request Sep 6, 2021
Merged
sipsorcery
sipsorcery reviewed Sep 6, 2021
View reviewed changes
@hebasto
Copy link
Member Author

hebasto commented Sep 7, 2021

Rebased 0f56ce3 -> 391a377 (pr22890.06 -> pr22890.07) due to the conflict with #21551.

maflcko pushed a commit that referenced this pull request Sep 7, 2021
Merge #22899: ci: Build and cache static Qt instead of downloading a …
ecf580e 
…pre-built one

3a68546 ci: Build and cache static Qt instead of downloading a pre-built one (Hennadii Stepanov)

Pull request description:

  This PR makes the MSVC build CI task free of [pre-built static Qt binaries](https://github.com/sipsorcery/qt_win_binary/releases). It uses the approach which is documented in #22890.

  It takes about 13 minutes to build a static Qt dependency (for 8 CPUs):

  ![Screenshot from 2021-09-06 08-59-08](https://user-images.githubusercontent.com/32963518/132167857-bce49c74-f258-4468-b45b-75d0cf3c670c.png)

  with the maximum total time:

  ![Screenshot from 2021-09-06 08-59-26](https://user-images.githubusercontent.com/32963518/132167881-b84bd4de-38cc-4cb1-b9f7-35642cbea8cc.png)

  There is an additional benefit of this PR. It is no longer required to build a new static Qt package when a CI Windows image upgrades its building tools, and breaks the compatibility with the recent Qt package.

ACKs for top commit:
  sipsorcery:
    utACK 3a68546.

Tree-SHA512: 2cf358ccecb26293b52c04158d6d3366ae6257cc3c04262e02234f7d7a03086885c67f0aad5702fcaa6f035fe4a09967a81245c561614875ecd2e90e2e00bbaa
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 7, 2021
Merge bitcoin#22899: ci: Build and cache static Qt instead of downloa…
37c78cb 
…ding a pre-built one

3a68546 ci: Build and cache static Qt instead of downloading a pre-built one (Hennadii Stepanov)

Pull request description:

  This PR makes the MSVC build CI task free of [pre-built static Qt binaries](https://github.com/sipsorcery/qt_win_binary/releases). It uses the approach which is documented in bitcoin#22890.

  It takes about 13 minutes to build a static Qt dependency (for 8 CPUs):

  ![Screenshot from 2021-09-06 08-59-08](https://user-images.githubusercontent.com/32963518/132167857-bce49c74-f258-4468-b45b-75d0cf3c670c.png)

  with the maximum total time:

  ![Screenshot from 2021-09-06 08-59-26](https://user-images.githubusercontent.com/32963518/132167881-b84bd4de-38cc-4cb1-b9f7-35642cbea8cc.png)

  There is an additional benefit of this PR. It is no longer required to build a new static Qt package when a CI Windows image upgrades its building tools, and breaks the compatibility with the recent Qt package.

ACKs for top commit:
  sipsorcery:
    utACK 3a68546.

Tree-SHA512: 2cf358ccecb26293b52c04158d6d3366ae6257cc3c04262e02234f7d7a03086885c67f0aad5702fcaa6f035fe4a09967a81245c561614875ecd2e90e2e00bbaa
@hebasto
build: Make <QtBaseDir> default name Qt and VS versions agnostic
5e42f2a 
This change allows users to not patch `common.qt.init.vcxproj` to fit
their Qt and Visual Studio versions. Also it simplifies code base
maintaining.

To specify a non-default path, the QTBASEDIR environment variable can be
used.
@hebasto
Copy link
Member Author

hebasto commented Sep 7, 2021

Rebased 391a377 -> 367203b (pr22890.07 -> pr22890.08) due to the conflict with #22899.

@hebasto hebasto mentioned this pull request Sep 8, 2021
Merged
@laanwj
Copy link
Member

laanwj commented Sep 9, 2021

that allow users to self-compile static Qt package which is required for building Bitcoin Core with Visual Studio.

Why is a static Qt package necessarily needed? I mean, on Linux it works fine to build it against a dynamic system Qt.

@hebasto
Copy link
Member Author

hebasto commented Sep 9, 2021

@laanwj

that allow users to self-compile static Qt package which is required for building Bitcoin Core with Visual Studio.

Why is a static Qt package necessarily needed? I mean, on Linux it works fine to build it against a dynamic system Qt.

A relevant discussion from #15529:

@NicolasDorier #15529 (comment):

Btw, would it be a good idea to use QT's precompiled dynamic library instead of the static lib?
We are using the msvc build mainly for debugging stuff so I don't think it would matter too much and prevent a bunch of headache.

@sipsorcery #15529 (comment)

@NicolasDorier nooooo :(. You don't want to know how long I've spent getting the Qt build working.

Putting aside the lost chunk of my life it would be a big change switching the build to dynamic linking. All the Bitcoin Core libraries would have to be built as dll's and there are then bound to be missing exports that will need to be added which would mean code changes.

I'd highly recommend sticking with static linking for consistency with the other Bitcoin Core builds. Hopefully one day the vcpkg static build of Qt will work and allow it to be treated the same as the other dependencies for the msvc build. See microsoft/vcpkg#4560 (comment).

@sipsorcery
Copy link
Contributor

sipsorcery commented Sep 9, 2021

Why is a static Qt package necessarily needed? I mean, on Linux it works fine to build it against a dynamic system Qt.

To further elaborate, to the best of my knowledge a static version of Qt is required for the Bitcoin Core version on Windows because we want the bitcoin-qt.exe and other executables to be statically linked against the C runtime (the MT compiler option).

The msvc compiler options page also has this quote:

All modules passed to a given invocation of the linker must have been compiled with the same run-time library compiler option (/MD, /MT, /LD).

I was never able to build bitcoim-qt.exe with a version of Qt compiled with the MD compiler option but perhaps I'm missing something.

@laanwj
Copy link
Member

laanwj commented Sep 16, 2021

OK, thanks for explaining, I was under the impression it was something new introduced here, but if it's simply the way things have always been…"it's a lot of extra work" is enough reason to not do it imo. FWIW, on Linux it's the othe way around, dynamically building is "go with the flow" whereas static is what requires a lot of extra work.

Concept ACK

fanquake
fanquake reviewed Sep 30, 2021
View reviewed changes
build_msvc/README.md Outdated
the default approach is to use the [vcpkg](https://docs.microsoft.com/en-us/cpp/vcpkg) package manager from Microsoft.

Options for installing the dependencies in a Visual Studio compatible manner are:
To install vcpkg:
Copy link
Member

@fanquake fanquake Sep 30, 2021

Choose a reason for hiding this comment

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

Please don't add instructions for installing a package manager here. It's got nothing todo with building Bitcoin Core, and is just another thing that can become outdated. Linking to upstream (https://github.com/Microsoft/vcpkg), where the instructions will always be correct, should be sufficient, and is what was being done previously.

Copy link
Member Author

@hebasto hebasto Sep 30, 2021

Choose a reason for hiding this comment

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

Thanks! Updated.

@hebasto
Copy link
Member Author

hebasto commented Sep 30, 2021

Updated 367203b -> 6bc4398 (pr22890.08 -> pr22890.09, diff):

@hebasto
Copy link
Member Author

hebasto commented Oct 6, 2021

@sipsorcery

Mind looking into this PR one more time?

@sipsorcery
Copy link
Contributor

sipsorcery commented Oct 6, 2021

ACK 6bc4398.

@fanquake fanquake merged commit a68de12 into bitcoin:master Oct 7, 2021
@hebasto hebasto deleted the 210904-msvc branch October 7, 2021 10:53
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 7, 2021
@fanquake
Merge bitcoin#22890: doc: Replace a link to Qt precompiled binaries w…
a38086d 
…ith com…
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@fanquake fanquake fanquake left review comments

+2 more reviewers

@sipsorcery sipsorcery sipsorcery left review comments

@jarolrod jarolrod jarolrod left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Build system Docs Windows

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@hebasto @DrahtBot @sipsorcery @fanquake @laanwj @jarolrod