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

test: Add missed header #729

Merged
merged 1 commit into from
May 17, 2023
Merged

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented May 16, 2023

Should fix MSVC link errors like that:

addressbooktests.obj : error LNK2019: unresolved external symbol "void __cdecl ConfirmMessage(class QString *,class std::chrono::duration<__int64,struct std::ratio<1,1000> >)" (?ConfirmMessage@@YAXPEAVQString@@V?$duration@_JU?$ratio@$00$0DOI@@std@@@chrono@std@@@Z) referenced in function "void __cdecl `anonymous namespace'::EditAddressAndSubmit(class EditAddressDialog *,class QString const &,class QString const &,class QString)" (?EditAddressAndSubmit@?A0x2e52698e@@YAXPEAVEditAddressDialog@@AEBVQString@@1V3@@Z) [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\test_bitcoin-qt\test_bitcoin-qt.vcxproj]
wallettests.obj : error LNK2001: unresolved external symbol "void __cdecl ConfirmMessage(class QString *,class std::chrono::duration<__int64,struct std::ratio<1,1000> >)" (?ConfirmMessage@@YAXPEAVQString@@V?$duration@_JU?$ratio@$00$0DOI@@std@@@chrono@std@@@Z) [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\test_bitcoin-qt\test_bitcoin-qt.vcxproj]
C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\x64\Release\test_bitcoin-qt.exe : fatal error LNK1120: 1 unresolved externals [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\test_bitcoin-qt\test_bitcoin-qt.vcxproj]

@DrahtBot
Copy link
Contributor

DrahtBot commented May 16, 2023

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 fanquake

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #650 (Add Import to Wallet GUI by KolbyML)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@hebasto
Copy link
Member Author

hebasto commented May 16, 2023

cc @fanquake @sipsorcery

Should fix MSVC link errors.
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK 36e2d51

@DrahtBot DrahtBot mentioned this pull request May 17, 2023
2 tasks
@hebasto hebasto changed the title qt, test: Add missed header test: Add missed header May 17, 2023
@hebasto hebasto merged commit a75c77e into bitcoin-core:master May 17, 2023
@hebasto hebasto deleted the 230516-header branch May 17, 2023 08:41
@maflcko
Copy link
Contributor

maflcko commented May 17, 2023

Any understanding why or when this happened?

@fanquake
Copy link
Member

I think it was bitcoin/bitcoin#26715.

@maflcko
Copy link
Contributor

maflcko commented May 17, 2023

But CI passed on the pull and merge commit (https://github.com/bitcoin/bitcoin/runs/13481551706), no?

@hebasto
Copy link
Member Author

hebasto commented May 17, 2023

The MSVC link issue was kind of intermittent. I guess, it was dependent on the actual order of processing of the source files.

@maflcko
Copy link
Contributor

maflcko commented May 17, 2023

Ok, interesting. Maybe with iwyu this kind of bug will be fixed eventually 🥲

@fanquake
Copy link
Member

Yea. I guess the compiler is broken? Developers should not have to worry about putting files in the "right order" for it.

@hebasto
Copy link
Member Author

hebasto commented May 17, 2023

Observing the same issue -- https://cirrus-ci.com/task/6646912535756800

:(

@maflcko
Copy link
Contributor

maflcko commented May 17, 2023

Anything holding back bitcoin/bitcoin#27571 ?

@fanquake
Copy link
Member

No.

@maflcko
Copy link
Contributor

maflcko commented May 17, 2023

Looks like iwyu doesn't work with qt?

(qt/test/util.h has correct #includes/fwd-decls)

qt/test/util.cpp should add these lines:
#include <QtCore/qobjectdefs.h>  // for FunctionPointer<>::IsPointerToMember...
#include <qapplication.h>        // for QApplication
#include <qlist.h>               // for QList
#include <qmessagebox.h>         // for QMessageBox
#include <qobject.h>             // for qobject_cast
#include <qpushbutton.h>         // for QPushButton
#include <qstring.h>             // for QString
#include <qtimer.h>              // for QTimer
#include <qwidget.h>             // for QWidget
#include <utility>               // for move

qt/test/util.cpp should remove these lines:
- #include <QApplication>  // lines 9-9
- #include <QMessageBox>  // lines 10-10
- #include <QPushButton>  // lines 11-11
- #include <QString>  // lines 12-12
- #include <QTimer>  // lines 13-13
- #include <QWidget>  // lines 14-14

@fanquake
Copy link
Member

Looks like iwyu doesn't work with qt?

We might have to provide a mapping file, for it to work sanely? i.e https://github.com/include-what-you-use/include-what-you-use/blob/master/qt5_11.imp.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 17, 2023
36e2d51 qt, test: Add missed header (Hennadii Stepanov)

Pull request description:

  Should fix MSVC link errors like [that](https://api.cirrus-ci.com/v1/task/4870882892447744/logs/build.log):
  ```
  addressbooktests.obj : error LNK2019: unresolved external symbol "void __cdecl ConfirmMessage(class QString *,class std::chrono::duration<__int64,struct std::ratio<1,1000> >)" (?ConfirmMessage@@YAXPEAVQString@@v?$duration@_JU?$ratio@$00$0DOI@@std@@@chrono@std@@@z) referenced in function "void __cdecl `anonymous namespace'::EditAddressAndSubmit(class EditAddressDialog *,class QString const &,class QString const &,class QString)" (?EditAddressAndSubmit@?A0x2e52698e@@YAXPEAVEditAddressDialog@@AEBVQString@@1v3@@z) [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\test_bitcoin-qt\test_bitcoin-qt.vcxproj]
  wallettests.obj : error LNK2001: unresolved external symbol "void __cdecl ConfirmMessage(class QString *,class std::chrono::duration<__int64,struct std::ratio<1,1000> >)" (?ConfirmMessage@@YAXPEAVQString@@v?$duration@_JU?$ratio@$00$0DOI@@std@@@chrono@std@@@z) [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\test_bitcoin-qt\test_bitcoin-qt.vcxproj]
  C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\x64\Release\test_bitcoin-qt.exe : fatal error LNK1120: 1 unresolved externals [C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\build_msvc\test_bitcoin-qt\test_bitcoin-qt.vcxproj]
  ```

ACKs for top commit:
  fanquake:
    ACK 36e2d51

Tree-SHA512: 84685598fbf8857c0284ff660d953b93da3c2f47ba4ac0d3591b5009a6bcdb76898031fd70f289c4256ce389e485bd259ca145f9f862f085795e374dfa88705d
@hebasto
Copy link
Member Author

hebasto commented May 21, 2023

Looks like iwyu doesn't work with qt?

We might have to provide a mapping file, for it to work sanely? i.e include-what-you-use/include-what-you-use@master/qt5_11.imp.

Added in bitcoin/bitcoin#27710.

@bitcoin-core bitcoin-core locked and limited conversation to collaborators May 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants