Depends : Qt Use Top-Level Structure #20600

pull BlockMechanic wants to merge 16 commits into bitcoin:master from BlockMechanic:add-qml-depends changing 5 files +824 −29
  1. BlockMechanic commented at 1:30 pm on December 8, 2020: contributor

    This PR reworks the way Qt dependency is built :-

    1. Introduces a top level config/build which
    2. Simplifies the way Qt is built, removing confusing lines
    3. Resolves static build for #16883
    4. Mimic how Qt is built from repo/all-in-one
    5. Is related to bitcoin#20504 which makes (q)make commands simpler
    6. Continuation/Completion of https://github.com/icota/bitcoin/commit/683710e072eb90296ae5a57f53bce7cac799fc41 which had incomplete dependency chain.

    While trying to add Qml/QtQuick i took a closer look at how QT builds and found that it uses a top level qt.pro which by far simplifies the build process.

  2. Add QML/ QtQuick Depends ef283d513d
  3. Start using top level Qt for depends 633660c529
  4. fanquake added the label Build system on Dec 8, 2020
  5. MarcoFalke commented at 1:54 pm on December 8, 2020: member
    The ci doesn’t pass, but this looks like a major rewrite, so it would be better to wait for some Concept ACKs from the build people
  6. Update qt.mk 806830087d
  7. sipa commented at 6:27 pm on December 8, 2020: member

    so it would be better to wait for some Concept ACKs from the build people

    Seven, to the Build People, great miners and craftsmen of the mountain halls.

  8. DrahtBot commented at 7:31 pm on December 8, 2020: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #20650 (depends: Drop workaround for a fixed bug in Qt build system by hebasto)
    • #20642 (depends: Drop unneeded patches for qt package by hebasto)
    • #20641 (depends: Use Qt top-level build facilities by hebasto)
    • #20504 (build: use more legible (q)make commands in qt package by fanquake)

    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.

  9. Update funcs.mk 
    Temp revert, #19882
    7eb9c96fc9
  10. Update qt.mk 
    Remove Debug line
    4ff303e507
  11. Delete fix_android_base_head_conf.patch 
    Removed unused patch
    88da239525
  12. jonasschnelli commented at 7:46 am on December 9, 2020: contributor
    Concept ACK on adding support for Qml/QtQuick. Haven’t studied the code in deep but the submodule part seems messy.
  13. Version Agnostic f7aefca635
  14. Remove unnecessary line e96fb4a362
  15. BlockMechanic referenced this in commit 683710e072 on Dec 10, 2020
  16. Rename to qt.modules f295c9d655
  17. in depends/patches/qt/.gitmodules:1 in 88da239525 outdated 
    0@@ -0,0 +1,50 @@
1+[submodule "qtbase"]
    laanwj commented at 1:22 pm on December 10, 2020:
    I’m not entirely sure what this does. Does this make qt submodules of our repository?
    BlockMechanic commented at 1:34 pm on December 10, 2020:
    Hi, No, it does not make qt submodule of bitcoin. The file is just used by qt.pro to determine which qt modules to build, and what their dependencies are. Perhaps, i can rename the file to prevent confusion ?
    BlockMechanic commented at 1:57 pm on December 10, 2020:
    @laanwj I’ve renamed the files to prevent confusion
  18. icota commented at 1:55 pm on December 10, 2020: contributor

    Concept ACK but:

    • changing the way we build Qt and (subsequent) enabling of QML modules should probably be two separate PRs
    • .configure is a script so doesn’t really belong in patches (actually none of the added files do)
  19. BlockMechanic commented at 2:04 pm on December 10, 2020: contributor

    Concept ACK but:

    • changing the way we build Qt and (subsequent) enabling of QML modules should probably be two separate PRs
    • .configure is a script so doesn’t really belong in patches (actually none of the added files do)

    Yeah, none of them are strictly patches. I was avoiding making a separate folder to maintain general logic/structure.

  20. in depends/patches/qt/.configure:1 in 88da239525 outdated 
    0@@ -0,0 +1,17 @@
1+#! /bin/sh
    icota commented at 2:12 pm on December 10, 2020:
    This files looks to be stripped of its licence (LGPL). It’s better to extract the file (and others) directly from the submodules. If the files have been slightly modified from the Qt source add a patchfile - to be in line with the current practice.
    BlockMechanic commented at 2:53 pm on December 10, 2020:
    These specific files are not within the submodules unfortunately nor are they listed in the download directory see :- https://download.qt.io/official_releases/qt/5.9/5.9.8/submodules. They are only in the all-in-one or the git, so I had copied them separately to avoid downloading the whole all-in-one only to extract these three files and then duplicate the work by separately downloading each module.
    icota commented at 5:34 pm on December 10, 2020:
    In that case I vote for downloading the all-in-one. It would reduce this PR by three files and most of its lines. It would also simplify existing code. The download would take a bit longer but if we want QML I think it’s worth it.
    promag commented at 5:53 pm on December 10, 2020:
    I’m already working on that approach @icota. Also worth noting that single file was the old approach, and it was changed in ab67dd7818ff2d0910f3fd9bfca9412d85de4424, maybe @theuni had a motivation for using modules.
    BlockMechanic commented at 7:26 pm on December 10, 2020:
    @promag I have an all in one build already functional, should i open a PR and we collaborate ?
    promag commented at 7:45 pm on December 10, 2020:
    @BlockMechanic sure! Happy to review and compare with my change.
  21. icota changes_requested
  22. Add complete configure file 37b7460313
  23. hebasto commented at 6:14 pm on December 10, 2020: member
    Concept ACK on adding QML support.
  24. Create qt (copy).mk 6f500bbf23
  25. Update qt.mk a606ebd17a
  26. Delete qt (copy).mk 158b0a9c14
  27. Resolve Modules b02b2a871f
  28. Resolve Modules 68b981618b
  29. Resolve Modules 96306533a3
  30. hebasto commented at 2:34 pm on December 13, 2020: member

    @icota

    • changing the way we build Qt and (subsequent) enabling of QML modules should probably be two separate PRs

    The first part is implemented in #20641.

  31. DrahtBot commented at 1:44 pm on December 16, 2020: member

    🐙 This pull request conflicts with the target branch and needs rebase.

    Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

  32. DrahtBot added the label Needs rebase on Dec 16, 2020
  33. fanquake commented at 6:03 am on December 17, 2020: member
    I’m not a fan of this approach. It’s also unclear exactly what’s going on, or why this is being done this way from a quick glance, as most of the commits have no useful title or description. I’m going to close this in favour of #20641, as it seems that achieves the same goal, and is going to be far more maintainable in the long term.
  34. fanquake closed this on Dec 17, 2020
  35. fanquake referenced this in commit 4315dc02a1 on Jul 18, 2021
  36. sidhujag referenced this in commit 4a59fda74b on Jul 23, 2021
  37. DrahtBot locked this on Feb 15, 2022


BlockMechanic MarcoFalke sipa DrahtBot jonasschnelli laanwj icota promag hebasto fanquake

Labels
Build system Needs rebase

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me