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.

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

Avoid non-self-contained Windows header #789

Merged
merged 1 commit into from
Jan 26, 2024
Merged

Avoid non-self-contained Windows header #789

merged 1 commit into from
Jan 26, 2024

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Jan 19, 2024

Using the windows.h header guarantees correctness regardless of the content of other headers.

For more details, please refer to https://stackoverflow.com/questions/4845198/fatal-error-no-target-architecture-in-visual-studio

Fixes the MSVC build when using the upcoming CMake-based build system and Qt packages installed via the vcpkg package manager.

Related to hebasto/bitcoin#77.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 19, 2024 •
edited
Loading

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK theuni
Stale ACK sipsorcery

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@hebasto hebasto mentioned this pull request Jan 19, 2024
Merged
@hebasto
Copy link
Member Author

hebasto commented Jan 19, 2024

Friendly ping @sipsorcery :)

@sipsorcery
Copy link
Contributor

utACK c95767a.

@theuni
Copy link
Contributor

theuni commented Jan 22, 2024

I don't disagree with using self-contained headers, but according to your link it should only be necessary if we don't define WIN32_LEAN_AND_MEAN, which we do.

So, this makes me think there must be some compilation units where this somehow doesn't get defined?

@hebasto
Copy link
Member Author

hebasto commented Jan 22, 2024

So, this makes me think there must be some compilation units where this somehow doesn't get defined?

I suppose, they might be the qt* stuff built by vcpkg.

@hebasto
Copy link
Member Author

hebasto commented Jan 22, 2024

@theuni

You might be interested to consider the following example: https://godbolt.org/z/b8rns1e6x

fanquake
fanquake reviewed Jan 23, 2024
View reviewed changes
src/qt/winshutdownmonitor.h Outdated
@@ -10,7 +10,7 @@
#include <QString>
#include <functional>

#include <windef.h> // for HWND
#include <windows.h> // for HWND
Copy link
Member

Choose a reason for hiding this comment

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

Comment should be removed? We are removing these "for xyz" comments in any case, but this one would also now be incorrect.

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment should be removed?

Thanks! Done.

@hebasto
qt: Avoid non-self-contained Windows header
8023640 
Using the `windows.h` header guarantees correctness regardless of the
content of other headers.
For more details, please refer to https://stackoverflow.com/questions/4845198/fatal-error-no-target-architecture-in-visual-studio

Fixes the MSVC build when using the upcoming CMake-based build system
and Qt packages installed via the vcpkg package manager.
@theuni
Copy link
Contributor

theuni commented Jan 26, 2024

ACK 8023640. It's not completely clear to me why this currently works, but I don't think it's worth wasting more time on. windows.h seems more correct regardless.

@hebasto hebasto merged commit fa2bcf6 into bitcoin-core:master Jan 26, 2024
16 checks passed
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 24, 2024
@hebasto @PastaPastaPasta
Merge bitcoin-core/gui#789: Avoid non-self-contained Windows header
e06de98 
8023640 qt: Avoid non-self-contained Windows header (Hennadii Stepanov)

Pull request description:

  Using the `windows.h` header guarantees correctness regardless of the content of other headers.

  For more details, please refer to https://stackoverflow.com/questions/4845198/fatal-error-no-target-architecture-in-visual-studio

  Fixes the MSVC build when using the upcoming CMake-based build system and Qt packages installed via the vcpkg package manager.

  Related to hebasto#77.

ACKs for top commit:
  theuni:
    ACK 8023640. It's not completely clear to me why this currently works, but I don't think it's worth wasting more time on. `windows.h` seems more correct regardless.

Tree-SHA512: 1c03f909943111fb2663f86d33ec9a947bc5903819e5bd94f436f6b0782d9f5c5d80d9cd3490674ecd8921b2981c509e97e41580bccc436f8b5c7db84b4e493c
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 24, 2024
@hebasto @PastaPastaPasta
Merge bitcoin-core/gui#789: Avoid non-self-contained Windows header
ea17d83 
8023640 qt: Avoid non-self-contained Windows header (Hennadii Stepanov)

Pull request description:

  Using the `windows.h` header guarantees correctness regardless of the content of other headers.

  For more details, please refer to https://stackoverflow.com/questions/4845198/fatal-error-no-target-architecture-in-visual-studio

  Fixes the MSVC build when using the upcoming CMake-based build system and Qt packages installed via the vcpkg package manager.

  Related to hebasto#77.

ACKs for top commit:
  theuni:
    ACK 8023640. It's not completely clear to me why this currently works, but I don't think it's worth wasting more time on. `windows.h` seems more correct regardless.

Tree-SHA512: 1c03f909943111fb2663f86d33ec9a947bc5903819e5bd94f436f6b0782d9f5c5d80d9cd3490674ecd8921b2981c509e97e41580bccc436f8b5c7db84b4e493c
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 24, 2024
@hebasto @PastaPastaPasta
Merge bitcoin-core/gui#789: Avoid non-self-contained Windows header
2a77808 
8023640 qt: Avoid non-self-contained Windows header (Hennadii Stepanov)

Pull request description:

  Using the `windows.h` header guarantees correctness regardless of the content of other headers.

  For more details, please refer to https://stackoverflow.com/questions/4845198/fatal-error-no-target-architecture-in-visual-studio

  Fixes the MSVC build when using the upcoming CMake-based build system and Qt packages installed via the vcpkg package manager.

  Related to hebasto#77.

ACKs for top commit:
  theuni:
    ACK 8023640. It's not completely clear to me why this currently works, but I don't think it's worth wasting more time on. `windows.h` seems more correct regardless.

Tree-SHA512: 1c03f909943111fb2663f86d33ec9a947bc5903819e5bd94f436f6b0782d9f5c5d80d9cd3490674ecd8921b2981c509e97e41580bccc436f8b5c7db84b4e493c
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Oct 24, 2024
@PastaPastaPasta
Merge #6346: backport: trivial 2024 10 23 pr2
a673193 
8bf1d06 Merge bitcoin#29308: doc: update `BroadcastTransaction` comment (glozow)
2a77808 Merge bitcoin-core/gui#789: Avoid non-self-contained Windows header (Hennadii Stepanov)
da371b8 Merge bitcoin#28870: depends: Include `config.guess` and `config.sub` into `meta_depends` (fanquake)
2e41562 Merge bitcoin#29219: fuzz: Improve fuzzing stability for ellswift_roundtrip harness (fanquake)
b091329 Merge bitcoin#29211: fuzz: fix `connman` initialization (Ava Chow)
df42d41 Merge bitcoin#29200: net: create I2P sessions using both ECIES-X25519 and ElGamal encryption (fanquake)
4cdd1a8 Merge bitcoin#29172: fuzz: set `nMaxOutboundLimit` in connman target (fanquake)
97012ea Merge bitcoin#28962: doc: Rework guix docs after 1.4 release (fanquake)
c70ff5d Merge bitcoin#28844: contrib: drop GCC MAX_VERSION to 4.3.0 in symbol-check (fanquake)
e6f19e7 Merge bitcoin#29068: test: Actually fail when a python unit test fails (fanquake)
75e0334 Merge bitcoin#28989: test: Fix test by checking the actual exception instance (Andrew Chow)
8cd85d3 Merge bitcoin#28852: script, assumeutxo: Enhance validations in utxo_snapshot.sh (Ryan Ofsky)
fd2e88d Merge bitcoin#26077: guix: switch from `guix environment` to `guix shell` (fanquake)
02741a7 Merge bitcoin#28913: coins: make sure PoolAllocator uses the correct alignment (fanquake)
dfd53da Merge bitcoin#28902: doc: Simplify guix install doc, after 1.4 release (fanquake)

Pull request description:

  ## Issue being fixed or feature implemented
  Batch of trivial backports

  ## What was done?
  See commits

  ## How Has This Been Tested?
  built locally; large combined merge passed tests locally

  ## Breaking Changes
  Should be none

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK 8bf1d06
  knst:
    utACK 8bf1d06

Tree-SHA512: 506273e5a188f9ca74edf656e3cd338992192e6e97f68c89fc43e34be20fb7f211b48e4dfa8693727839a7920da8284509413c722f55774a428939c296dad517
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fanquake fanquake fanquake left review comments

@sipsorcery sipsorcery Awaiting requested review from sipsorcery

Assignees
No one assigned
Labels
Windows
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hebasto @DrahtBot @sipsorcery @theuni @fanquake