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

scripted-diff: Avoid incompatibility with CMake AUTOUIC feature #25338

Merged
merged 1 commit into from
Jun 15, 2022

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Jun 11, 2022

Working on migration from Autotools to CMake build system, I found that our current code base needs to be adjusted.

CMake allows to

handle the Qt uic code generator automatically

When using this feature, statements like #include "ui_<ui_base>.h" are processed in a special way.

The node/ui_interface.h unintentionally breaks this feature. Of course, it is possible to provide a list of source files to be excluded from AUTOUIC. But, unfortunately, this approach does not work for the qt/sendcoinsdialog.cpp source file, where there are both

#include <qt/forms/ui_sendcoinsdialog.h>
and
#include <node/ui_interface.h>

@dongcarl
Copy link
Contributor

Oh! What happens if we do set_property(SOURCE node/ui_interface.h PROPERTY SKIP_AUTOUIC ON)?

@hebasto
Copy link
Member Author

hebasto commented Jun 11, 2022

@dongcarl

Oh! What happens if we do set_property(SOURCE node/ui_interface.h PROPERTY SKIP_AUTOUIC ON)?

It does not help:

[ 12%] Automatic MOC and UIC for target bitcoinqt

AutoUic error
-------------
"SRC:/src/qt/sendcoinsdialog.cpp"
includes the uic file "node/ui_interface.h",
but the user interface file "interface.ui"
could not be found in the following directories
  "SRC:/src/qt/node"
  "SRC:/src/qt"
  "SRC:/src/qt/forms"
  "SRC:/src/qt/forms/node"

gmake[3]: *** [src/qt/CMakeFiles/bitcoinqt_autogen.dir/build.make:71: src/qt/CMakeFiles/bitcoinqt_autogen] Error 1
gmake[2]: *** [CMakeFiles/Makefile2:754: src/qt/CMakeFiles/bitcoinqt_autogen.dir/all] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:734: src/qt/CMakeFiles/bitcoin-qt.dir/rule] Error 2
gmake: *** [Makefile:358: bitcoin-qt] Error 2

@DrahtBot
Copy link
Contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)
  • #23443 (p2p: Erlay support signaling by naumenkogs)

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 Jun 13, 2022

Friendly ping @ryanofsky

Copy link
Member

@furszy furszy left a comment

Choose a reason for hiding this comment

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

Concept ACK

src/Makefile.am Outdated
@@ -198,7 +198,7 @@ BITCOIN_CORE_H = \
node/minisketchwrapper.h \
node/psbt.h \
node/transaction.h \
node/ui_interface.h \
node/uiinterface.h \
Copy link
Member

Choose a reason for hiding this comment

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

interface_ui.h is a bit more readable

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Updated.

-BEGIN VERIFY SCRIPT-
sed -i "s|node/ui_interface|node/interface_ui|g" $(git grep -l "node/ui_interface" ./src)
git mv src/node/ui_interface.cpp src/node/interface_ui.cpp
git mv src/node/ui_interface.h src/node/interface_ui.h
sed -i "s|BITCOIN_NODE_UI_INTERFACE_H|BITCOIN_NODE_INTERFACE_UI_H|g" src/node/interface_ui.h
-END VERIFY SCRIPT-
@laanwj
Copy link
Member

laanwj commented Jun 14, 2022

I don't know, ~0 on this, I dislike the idea of having to rename source files to make the build system happy.

Oh! What happens if we do set_property(SOURCE node/ui_interface.h PROPERTY SKIP_AUTOUIC ON)?

I would prefer a solution like that.

@maflcko
Copy link
Member

maflcko commented Jun 14, 2022

cr ACK 018d70b

New name is fine and I reviewed the scripted diff.

No opinion on the build system stuff.

@hebasto
Copy link
Member Author

hebasto commented Jun 14, 2022

@laanwj

I don't know, ~0 on this, I dislike the idea of having to rename source files to make the build system happy.

Oh! What happens if we do set_property(SOURCE node/ui_interface.h PROPERTY SKIP_AUTOUIC ON)?

I would prefer a solution like that.

You are right. I also prefer some kind of build system switchers. But they do not work for qt/sendcoinsdialog.cpp source file, which should be processed, as it contains #include <qt/forms/ui_sendcoinsdialog.h> , and should be skipped, as it contains #include <node/ui_interface.h>. Looks like it is impossible without renaming.

@Sjors
Copy link
Member

Sjors commented Jun 14, 2022

We already have build system code that special cases ui_<...>.h files (though afaik only in qt/forms), so the rename is probably a good idea anyway.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Code review ACK 018d70b

Copy link
Member

@furszy furszy left a comment

Choose a reason for hiding this comment

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

Code review ACK 018d70b

Small scripted-diff.

@maflcko maflcko merged commit 38c63e3 into bitcoin:master Jun 15, 2022
@hebasto hebasto deleted the 220611-autouic branch June 15, 2022 06:27
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 15, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Jun 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants