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

doc: Document ALLOW_HOST_PACKAGES dependency option #19124

Merged
merged 1 commit into from Oct 27, 2020

Conversation

skmcontrib
Copy link

Provided entry in depends, README.md to ensure that ALLOW_HOST_PACKAGES dependency option is documented, #19113

@hebasto
Copy link
Member

hebasto commented May 31, 2020

@skmcontrib Thank you for working on it.

cc @dongcarl

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 11, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

@@ -95,6 +95,7 @@ The following can be set when running make: make FOO=bar
DEBUG: disable some optimizations and enable more runtime checking
HOST_ID_SALT: Optional salt to use when generating host package ids
BUILD_ID_SALT: Optional salt to use when generating build package ids
ALLOW_HOST_PACKAGE: compile using hosts library set (via pkg-config)
Copy link
Member

Choose a reason for hiding this comment

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

Concept ACK.

@skmcontrib
Could you emphasize that the host's packages are used when they are not built due to the NO_* variable set?

Also this line could be moved up to be just under the NO_UPNP line.

Copy link
Member

@laanwj laanwj Jul 22, 2020

Choose a reason for hiding this comment

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

Can you please elaborate a bit further what this means? What is compiled using hosts libraries? And which host libraries, which packages are replaced by host packages? When would one want to use this option?

@@ -95,6 +95,7 @@ The following can be set when running make: make FOO=bar
DEBUG: disable some optimizations and enable more runtime checking
HOST_ID_SALT: Optional salt to use when generating host package ids
BUILD_ID_SALT: Optional salt to use when generating build package ids
ALLOW_HOST_PACKAGE: compile using hosts library set (via pkg-config)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ALLOW_HOST_PACKAGE: compile using hosts library set (via pkg-config)
ALLOW_HOST_PACKAGES: compile using hosts library set (via pkg-config)

@hebasto
Copy link
Member

hebasto commented Jul 23, 2020

@skmcontrib
Mind rebasing and addressing reviewers' comments?

@skmcontrib
Copy link
Author

Thanks @hebasto. Will look

@hebasto
Copy link
Member

hebasto commented Jul 26, 2020

@skmcontrib Mind squashing into one commit?

@skmcontrib
Copy link
Author

@hebasto @dongcarl @laanwj Thanks for your review comments. I have incorporated all the changes.

@hebasto
Copy link
Member

hebasto commented Aug 4, 2020

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

@skmcontrib
Copy link
Author

@hebasto The earlier squash I did merged all my previous commits into a single commit. So now there are two commits - a squashed commit (e8c3e2b) and also a merge commit(e4946cb). I'm having some trouble squashing these two. Maybe I don't understand this fully. Any pointers? Also as suggested in the contributing guidelines I have enabled "allow edits from maintainers". Sorry for the trouble and thanks for the help.

@hebasto
Copy link
Member

hebasto commented Aug 9, 2020

@skmcontrib

@hebasto The earlier squash I did merged all my previous commits into a single commit. So now there are two commits - a squashed commit (e8c3e2b) and also a merge commit(e4946cb). I'm having some trouble squashing these two. Maybe I don't understand this fully. Any pointers? Also as suggested in the contributing guidelines I have enabled "allow edits from maintainers". Sorry for the trouble and thanks for the help.

You could start from this GitHub guide.

Your working branch in your forked repo should contain no merge commits. Only commits with your changes, that are based on "master" branch at some point, are allowed. The merge commit is made by a maintainer when your working branch is merged into this repo "master" branch.

"allow edits from maintainers" allows maintainers to edit PR's title and posts. It is a contributor's burden to keep his own working branch in ready-to-merge state.

Comment on lines 105 to 107
<dd>Enables the hosts packages in case automatic download/build of dependencies
is disabled via the NO_* options (example NO_QT etc). This is useful in case of
cross-compilation (example: compilation for macOS on debian)</dd>
Copy link
Member

@hebasto hebasto Aug 9, 2020

Choose a reason for hiding this comment

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

Two notes to consider:

Suggested change
<dd>Enables the hosts packages in case automatic download/build of dependencies
is disabled via the NO_* options (example NO_QT etc). This is useful in case of
cross-compilation (example: compilation for macOS on debian)</dd>
<dd>Packages that are missed in dependencies (due to `NO_*` option or
build script logic) are searched for among the host system packages using
`pkg-config`. It allows to build with packages of other (newer) versions</dd>

Copy link
Author

Choose a reason for hiding this comment

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

@hebasto Hopefully I got it right this time. Sorry for being dense and the delay. Appreciate your help and the good learning.

@skmcontrib
Copy link
Author

@hebasto incorporated all the review comments. Thanks

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 47e2a35.

nit: it appears that back quoting doesn't work within <dd> ... </dd>, therefore it could be dropped

@hebasto
Copy link
Member

hebasto commented Oct 6, 2020

friendly ping @dongcarl

@laanwj
Copy link
Member

laanwj commented Oct 27, 2020

ACK 47e2a35

@laanwj laanwj merged commit c1564ba into bitcoin:master Oct 27, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 27, 2020
47e2a35 doc: Document ALLOW_HOST_PACKAGES dependency option (skmcontrib)

Pull request description:

  Provided entry in depends, README.md to ensure that ALLOW_HOST_PACKAGES dependency option is documented, bitcoin#19113

ACKs for top commit:
  hebasto:
    ACK 47e2a35.

Tree-SHA512: 10d9285885be25f092881e4886c6a804cd42b5224bdf1dfa8b8369463808ddaf533a8604f14f7fe45478434a22feae98053f4731b51d976c071d69882bdac72b
furszy added a commit to PIVX-Project/PIVX that referenced this pull request May 25, 2021
e1b89ac Fix QPainter non-determinism on macOS (Andrew Chow)
831c317 macOS deploy: use the new plistlib API (Jonas Schnelli)
5857aaf doc: Document ALLOW_HOST_PACKAGES dependency option (skmcontrib)
2329e08 build: Fix behavior when ALLOW_HOST_PACKAGES unset (Hennadii Stepanov)
1768870 depends: native_ds_store 1.3.0 (fanquake)
3f9f3e5 depends: pull upstream libdmg-hfsplus changes (fanquake)
f7606dc depends: latest config.guess & config.sub (fanquake)
cc3ae74 depends: bump native_cctools for fixed lto with external clang (Cory Fields)
b26c648 depends: enable lto support for Apple's ld64 (Cory Fields)
50933d7 depends: Add documentation for FORCE_USE_SYSTEM_CLANG make flag (Carl Dong)
ba3ddf2 depends: Reformat make options as definition list (Carl Dong)
3b855a7 depends: Add justifications for macOS clang flags (Carl Dong)
4104de0 depends: specify libc++ header location for darwin (Cory Fields)
cd4335f depends: force a new host id string if FORCE_USE_SYSTEM_CLANG is in use (Cory Fields)
d30e1af depends: Allow building with system clang (Carl Dong)
234828b depends: Decouple toolchain + binutils (Carl Dong)
1dd3a5a doc: explain why passing -mlinker-version is required (fanquake)
5cc0d0f darwin: pass mlinker-version so that clang enables new features (Cory Fields)
813a552 macos: Bump to xcode 11.3.1 and 10.15 SDK (Cory Fields)
ee7085f depends: bump MacOS toolchain (Cory Fields)
e5b092b contrib: macdeploy: Remove historical extraction notes (Carl Dong)
5893caf contrib: macdeploy: Use apple-sdk-tools instead of xar+pbzx (Carl Dong)
9f2d4ba native_cctools: Don't use libc++ from pinned clang (Carl Dong)
0c8d217 Adapt rest of tooling to new SDK naming scheme (Carl Dong)
bdacfa8 contrib: macdeploy: Correctly generate macOS SDK (Carl Dong)
f7eee2c Fix naming of macOS SDK and clarify version (Andrew Chow)
62f9e23 build: use macOS 10.14 SDK (fanquake)
bc2e1af depends: native_cctools 921, ld64 409.12, libtapi 1000.10.8 (fanquake)
a296d87 depends: clang 6.0.1 (fanquake)
8f6c475 build: Set minimum supported macOS to 10.12 (Fuzzbawls)

Pull request description:

  This backports the following upstream PRs to update the macOS cross-compiling tools:

  bitcoin#17550
  bitcoin#16392
  bitcoin#18589
  bitcoin#19240
  bitcoin#19407
  bitcoin#17919
  bitcoin#19530
  bitcoin#17057
  bitcoin#20333
  bitcoin#18051
  bitcoin#19124
  bitcoin#20298
  bitcoin#20447

  The tools being updated are

  ### Clang
  Upgraded from `3.7.1` to `8.0.0`

  ### cctools

  * cctools `877.8` -> `949.0.1`
  * LD64 `253.9` -> `530`
  * TAPI `1000.10.8`

  ### DSStore
  Upgraded from `1.1.2` to `1.3.0` (this removes the biplist dependency)

  This also effectively bumps our minimum supported macOS version to 10.12 (Sierra).

ACKs for top commit:
  furszy:
    tested ACK e1b89ac
  random-zebra:
    utACK e1b89ac

Tree-SHA512: f5cec8db57e07d8855070646b9e1400d48aac1d01e3c2c3b3e134665c6372d6535f3328888bb9a75087f7b3d5231ecb4b509723bfa51bd40770ffe2810c67f65
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
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.

None yet

7 participants