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

pupnp: Open enableReuseAddr configure option #93099

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

pupnp: Open enableReuseAddr configure option #93099

wants to merge 1 commit into from

Conversation

ardumont
Copy link
Contributor

@ardumont ardumont commented Jul 14, 2020 •
edited

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS,
    • macOS
    • other: debian with nix
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip" [1] [2]
  • Tested execution of all binary files (usually in ./result/bin/)
  • impact on package closure size: 8 bytes less: 33534240 (after, flag enabled), 33534248 (before or after with flag off, the default)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Both tested with the following to ensure the flip does take place (checking the configure output):

nix-build --expr "(import <nixpkgs> {}).callPackage ./pkgs/development/libraries/pupnp { enableReuseAddr = false; }"
nix-build --expr "(import <nixpkgs> {}).callPackage ./pkgs/development/libraries/pupnp { enableReuseAddr = true; }"

[1]

nixpkgs-review wip
$ git -c fetch.prune=false fetch --force https://github.com/NixOS/nixpkgs master:refs/nixpkgs-review/0
$ git worktree add /home/tony/.cache/nixpkgs-review/rev-8326dc9cff1059e31451de4979f13c6b222c5315-dirty/nixpkgs c00959877fb06b09468562518b408acda886c79e
Preparing worktree (detached HEAD c00959877fb)
Updating files: 100% (23364/23364), done.
HEAD is now at c00959877fb Merge pull request #105425 from r-ryantm/auto-update/python3.7-mac_alias
$ nix-env -f /home/tony/.cache/nixpkgs-review/rev-8326dc9cff1059e31451de4979f13c6b222c5315-dirty/nixpkgs -qaP --xml --out-path --show-trace
No diff detected, stopping review...
$ git worktree prune

[2]

nixpkgs-review pr 93099
$ git -c fetch.prune=false fetch --force https://github.com/NixOS/nixpkgs master:refs/nixpkgs-review/0 pull/93099/head:refs/nixpkgs-review/1
From https://github.com/NixOS/nixpkgs
 + ba9ade5c557...8326dc9cff1 refs/pull/93099/head -> refs/nixpkgs-review/1  (forced update)
$ git worktree add /home/tony/.cache/nixpkgs-review/pr-93099/nixpkgs c00959877fb06b09468562518b408acda886c79e
Preparing worktree (detached HEAD c00959877fb)
Updating files: 100% (23364/23364), done.
HEAD is now at c00959877fb Merge pull request #105425 from r-ryantm/auto-update/python3.7-mac_alias
$ nix-env -f /home/tony/.cache/nixpkgs-review/pr-93099/nixpkgs -qaP --xml --out-path --show-trace
$ git merge --no-commit 8326dc9cff1059e31451de4979f13c6b222c5315
Updating c00959877fb..8326dc9cff1
Fast-forward
 pkgs/development/libraries/pupnp/default.nix | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
$ nix-env -f /home/tony/.cache/nixpkgs-review/pr-93099/nixpkgs -qaP --xml --out-path --show-trace --meta
Nothing to be built.
https://github.com/NixOS/nixpkgs/pull/93099
$ nix-shell /home/tony/.cache/nixpkgs-review/pr-93099/shell.nix
...
[nix-shell:~/.cache/nixpkgs-review/pr-93099]$ exit
$ git worktree prune

@ardumont ardumont changed the title Libupnp add reuse addr option libupnp: add reuse addr option Jul 14, 2020
@ardumont ardumont force-pushed the libupnp-add-reuse-addr-option branch from 7ba6b9d to d8839b8 Compare July 14, 2020 09:14
@ardumont ardumont changed the title libupnp: add reuse addr option pupnp: Open enableReuseAddr configure option Jul 14, 2020
@ardumont
Copy link
Contributor Author

(rebased on latest master and drop the other commit about the version update)

@ardumont ardumont force-pushed the libupnp-add-reuse-addr-option branch from d8839b8 to 5fd4fc8 Compare July 14, 2020 15:13
@ardumont ardumont mentioned this pull request Jul 16, 2020
Merged
10 tasks
SuperSandro2000
SuperSandro2000 reviewed Nov 29, 2020
View reviewed changes
pkgs/development/libraries/pupnp/default.nix Outdated

stdenv.mkDerivation rec {
with stdenv;
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure.
Can you please clarify what's the problem with that writing (if any :)?

Copy link
Member

Choose a reason for hiding this comment

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

with should be applied to the smallest scope possible.

SuperSandro2000
SuperSandro2000 requested changes Nov 29, 2020
View reviewed changes
pkgs/development/libraries/pupnp/default.nix Outdated
@@ -14,6 +16,8 @@ stdenv.mkDerivation rec {

nativeBuildInputs = [ autoreconfHook pkg-config ];

configureFlags = lib.optionals enableReuseAddr "--enable-reuseaddr";
Copy link
Member

Choose a reason for hiding this comment

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

We can enable this by default if it does not require any additional inputs.

Copy link
Member

Choose a reason for hiding this comment

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

@ardumont Is there a reason to not enable it by default?

@SuperSandro2000
Copy link
Member

Result of nixpkgs-review pr 93099 run on x86_64-darwin 1

@ardumont
pupnp: Open enableReuseAddr configure option
8326dc9 
So applications (e.g. gerbera) that needs it can build against it.
@ardumont ardumont force-pushed the libupnp-add-reuse-addr-option branch from 5fd4fc8 to 8326dc9 Compare November 30, 2020 14:37
@ardumont
Copy link
Contributor Author

Updated accordingly and rebased on latest master.

@stale
Copy link

stale bot commented Jun 26, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 26, 2021
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank marked this pull request as draft March 25, 2024 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
2.status: merge conflict 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ardumont @SuperSandro2000 @wegank