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

build: Drop ALLOW_HOST_PACKAGES support in depends #29203

Merged
merged 1 commit into from
Jan 9, 2024

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Jan 8, 2024

The ALLOW_HOST_PACKAGES variable was introduced in #10508 "to speed up build and avoid timeout".

It is no longer the case for our CI infrastructure, which uses self- hosted persistent workers and depends caching.

In the current circumstances, it does not seem worth porting this feature to the upcoming CMake-based build system.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 8, 2024

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK fanquake, TheCharlatan
Concept ACK theuni

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:

  • #29091 ([28.x] build: Bump g++ minimum supported version to 11 by maflcko)

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.

The `ALLOW_HOST_PACKAGES` variable was introduced in bitcoin#10508 "to
speed up build and avoid timeout".

It is no longer the case for our CI infrastructure, which uses self-
hosted persistent workers and depends caching.

In the current circumstances, it does not seem worth porting this
feature to the upcoming CMake-based build system.
@fanquake
Copy link
Member

fanquake commented Jan 9, 2024

ACK 080763a - I can't imagine this option got any use outside our CI. It's also mostly just at odds with the idea of a self-contained dependency builder.

@theuni
Copy link
Member

theuni commented Jan 9, 2024

Concept ACK. Happy to have this gone.

Copy link
Contributor

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

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

ACK 080763a

@DrahtBot DrahtBot requested a review from theuni January 9, 2024 10:57
@fanquake fanquake merged commit b3b19be into bitcoin:master Jan 9, 2024
16 checks passed
@hebasto hebasto deleted the 240108-allow branch January 9, 2024 16:09
@@ -9,8 +9,8 @@ export LC_ALL=C.UTF-8
export CONTAINER_NAME=ci_native_qt5
export CI_IMAGE_NAME_TAG="docker.io/debian:bullseye"
# Use minimum supported python3.9 and gcc-10, see doc/dependencies.md
export PACKAGES="gcc-10 g++-10 python3-zmq qtbase5-dev qttools5-dev-tools libdbus-1-dev libharfbuzz-dev"
export DEP_OPTS="NO_QT=1 NO_UPNP=1 NO_NATPMP=1 DEBUG=1 ALLOW_HOST_PACKAGES=1 CC=gcc-10 CXX=g++-10"
Copy link
Member

Choose a reason for hiding this comment

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

nit: Forgot to adjust the cirrus yaml description of this task?

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 for #29218 :)

hebasto added a commit to hebasto/bitcoin that referenced this pull request Jan 18, 2024
b965507 fixup! ci: Test CMake edge cases (Hennadii Stepanov)
fb5023d fixup! cmake: Add cross-compiling support (Hennadii Stepanov)
84f94a8 fixup! build: Generate `share/toolchain.cmake` in depends (Hennadii Stepanov)

Pull request description:

  This PR mirrors changes from bitcoin#29203.

ACKs for top commit:
  TheCharlatan:
    ACK b965507

Tree-SHA512: edeea1943825032c64c95452d7e7b2662bb63dfabb1a35cdbf32136112822b07fc0fb558db942c0d53c0bf3ffaf1cb480169cabd028e3311b0aa9420ccc6e4d4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants