Skip to content

Conversation

@fanquake
Copy link
Member

@fanquake fanquake commented Oct 27, 2025

These platforms fail to compile with it removed.
macOS: #33675
BSDs / Illumos: hebasto/bitcoin-core-nightly#79.

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 27, 2025

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33714.

Reviews

See the guideline for information on the review process.

Type Reviewers
Stale ACK hebasto

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

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 f24ef2b.

@fanquake fanquake force-pushed the drop_some_platforms branch from f24ef2b to 6feaf36 Compare October 28, 2025 17:47
@fanquake fanquake changed the title random: clarify where the environ extern is needed random: scope the environ extern to macOS Oct 28, 2025
@DrahtBot DrahtBot changed the title random: scope the environ extern to macOS random: scope the environ extern to macOS Oct 28, 2025
@hebasto
Copy link
Member

hebasto commented Oct 28, 2025

Testing on some other platforms: hebasto/bitcoin-core-nightly#79.

@hebasto
Copy link
Member

hebasto commented Oct 29, 2025

Testing on some other platforms: hebasto/bitcoin-core-nightly#79.

Builds on the *BSD and illumos systems failed.

@fanquake fanquake force-pushed the drop_some_platforms branch from 6feaf36 to fcdbce4 Compare October 29, 2025 10:58
@fanquake fanquake changed the title random: scope the environ extern to macOS random: scope environ extern to macOS, BSDs and Illumos Oct 29, 2025
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 fcdbce4.

#endif

#ifndef WIN32
extern char** environ; // NOLINT(readability-redundant-declaration): Necessary on some platforms
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think the comment can be kept as-is, or just refer to "Necessary on the above platforms". Otherwise, it is just one more line to maintain.

Also, I wonder if this can just be checked at configure-time, but no strong opinion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Re-modified the comment. Not sure there is too much value in moving this to configure.

These platforms fail to compile with it removed.
@fanquake fanquake force-pushed the drop_some_platforms branch from fcdbce4 to 79d6f45 Compare October 29, 2025 15:14
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.

5 participants