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.

Sign up for GitHub

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

Jump to bottom

bzip2, zstd: Add enableStatic option #243161

Merged
merged 2 commits into from
Nov 7, 2023
Merged

bzip2, zstd: Add enableStatic option #243161

merged 2 commits into from
Nov 7, 2023

Conversation

nh2
Copy link
Contributor

@nh2 nh2 commented Jul 12, 2023

Re-open of #237563 for staging instead of master.

Description of changes

Supports #61575.

The fact that these libs currently force-disable shared libraries when static ones are enabled was found during working on nh2/static-haskell-nix#116.

FYI @jonathanlking @aherrmann

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

CC from #237563: @l0b0 @SuperSandro2000 @wegank

@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Jul 12, 2023
@nh2 nh2 mentioned this pull request Jul 12, 2023
Closed
13 tasks
@nh2 nh2 changed the title More enable static [staging] bzip2, zstd: Add enableStatic option Jul 12, 2023
@nh2 nh2 mentioned this pull request Jul 12, 2023
Closed
12 tasks
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux-stdenv This PR causes stdenv to rebuild labels Jul 13, 2023
@ofborg ofborg bot requested a review from Mic92 July 13, 2023 00:07
nh2 added a commit to nh2/nixpkgs that referenced this pull request Jul 13, 2023
@nh2
bzip2: Add enableStatic. See NixOS#61575
75378f1 
nixpkgs upstreaming PR: NixOS#243161
nh2 added a commit to nh2/nixpkgs that referenced this pull request Jul 13, 2023
@nh2
zstd: Add enableStatic. See NixOS#61575
40d2db0 
nixpkgs upstreaming PR: NixOS#243161
@SuperSandro2000 SuperSandro2000 changed the title [staging] bzip2, zstd: Add enableStatic option bzip2, zstd: Add enableStatic option Jul 23, 2023
@SuperSandro2000 SuperSandro2000 changed the title bzip2, zstd: Add enableStatic option bzip2, zstd: Add enableStatic option Jul 23, 2023
SuperSandro2000
SuperSandro2000 reviewed Jul 23, 2023
View reviewed changes
pkgs/tools/compression/bzip2/default.nix Outdated
Comment on lines 2 to 3
, enableStatic ? with stdenv.hostPlatform; isStatic || isCygwin
, disableShared ? false
Copy link
Member

@SuperSandro2000 SuperSandro2000 Jul 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Do we need to assert that not both are disabled?
I think we should name them both either enable or disable to be less confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should name them both either enable or disable to be less confusing

Done.

Do we need to assert that not both are disabled?

Probably not; currently that would result in no special flags being passed to ./configure, which is probably OK and maybe handy for somebody sometime.

SuperSandro2000
SuperSandro2000 reviewed Jul 23, 2023
View reviewed changes
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

Also the see nixpkgs#XXXXX should probably be moved into the commit message from the headline

@nh2 nh2 mentioned this pull request Sep 7, 2023
Closed
@nh2
Copy link
Contributor Author

nh2 commented Sep 7, 2023

Also the see nixpkgs#XXXXX should probably be moved into the commit message from the headline

Not sure about it; given how short it is, I find it easier to be able to click relevant issues directly from the Github history or git log --oneline without having to expand anything.

teggotic pushed a commit to teggotic/nixpkgs that referenced this pull request Sep 17, 2023
@nh2 @teggotic
bzip2: Add enableStatic. See NixOS#61575
8cbca2c 
nixpkgs upstreaming PR: NixOS#243161
teggotic pushed a commit to teggotic/nixpkgs that referenced this pull request Sep 17, 2023
@nh2 @teggotic
zstd: Add enableStatic. See NixOS#61575
6558345 
nixpkgs upstreaming PR: NixOS#243161
jonathanlking added a commit to heyhabito/nixpkgs that referenced this pull request Oct 20, 2023
@jonathanlking
Cherry-pick (and fix) PR: NixOS#243161
0be37f6
u-quark pushed a commit to u-quark/nixpkgs that referenced this pull request Oct 23, 2023
@nh2 @u-quark
bzip2: Add enableStatic. See NixOS#61575
c28b046 
nixpkgs upstreaming PR: NixOS#243161
u-quark pushed a commit to u-quark/nixpkgs that referenced this pull request Oct 23, 2023
@nh2 @u-quark
zstd: Add enableStatic. See NixOS#61575
f0933fc 
nixpkgs upstreaming PR: NixOS#243161
@jonathanlking
Copy link
Contributor

I’m very keen on this being merged (both for simplicity and eventually for caching) — is there anything else outstanding? I’m very happy to help in any way I can!

CC @l0b0 @SuperSandro2000

l0b0
l0b0 approved these changes Nov 2, 2023
View reviewed changes
Copy link
Contributor

@l0b0 l0b0 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@Artturin Artturin merged commit fbb3ff5 into NixOS:staging Nov 7, 2023
22 checks passed
@jonathanlking jonathanlking mentioned this pull request Jan 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonathanlking jonathanlking jonathanlking left review comments

@l0b0 l0b0 l0b0 approved these changes

@Mic92 Mic92 Awaiting requested review from Mic92

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

Assignees
No one assigned
Labels
6.topic: stdenv Standard environment 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 10.rebuild-linux-stdenv This PR causes stdenv to rebuild
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nh2 @jonathanlking @l0b0 @SuperSandro2000 @Artturin