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

hdhomerun-config-gui: install path correction and warning about libhdhomerun version added #123390

Merged
merged 1 commit into from Apr 25, 2022
Merged

hdhomerun-config-gui: install path correction and warning about libhdhomerun version added #123390

merged 1 commit into from Apr 25, 2022

Conversation

LouisDK1
Copy link
Contributor

Motivation for this change

Package failed to build correctly due to wrong install path.
Also same version of libhdhomerun is needed hence warning added.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • 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"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@LouisDK1 LouisDK1 changed the title Hdhomerun 20210224 hdhomerun-config-gui: install path correction and warning about libhdhomerun version added May 17, 2021
@LouisDK1
Copy link
Contributor Author

LouisDK1 commented May 17, 2021 •
edited

This package was built successfully without the extra comment which has been added as per documentation.

Can somebody tell me why this results in a conflict?

@LouisDK1 LouisDK1 force-pushed the hdhomerun_20210224 branch 2 times, most recently from fe791b7 to 3f58e6b Compare May 30, 2021 11:50
@LouisDK1
Copy link
Contributor Author

@SuperSandro2000 I've moved the comment and the package seems to build now.

@SuperSandro2000
Copy link
Member

@LouisDK1 please resolve the merge conflict

@LouisDK1
Copy link
Contributor Author

LouisDK1 commented Jun 9, 2021

@LouisDK1 please resolve the merge conflict

I changed the position of the comment as requested by you but apparently the build fails.

Should I place the comment somewhere else instead?

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/add-comment-without-getting-a-parse-error/13842/1

@SuperSandro2000
Copy link
Member

@LouisDK1 please resolve the merge conflict

I changed the position of the comment as requested by you but apparently the build fails.

Should I place the comment somewhere else instead?

it is fine there. Please resolve the new merge conflict.

@LouisDK1
Copy link
Contributor Author

@SuperSandro2000 There's still a merge conflict which seems to be the placement of the new comment: https://github.com/NixOS/nixpkgs/pull/123390/conflicts

Where you I put the comment to resolve it?

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/add-comment-without-getting-a-parse-error/13842/3

@SuperSandro2000
Copy link
Member

@SuperSandro2000 There's still a merge conflict which seems to be the placement of the new comment: #123390 (conflicts)

Where you I put the comment to resolve it?

rebase the PR and resolve the merge conflict.

@LouisDK1 LouisDK1 force-pushed the hdhomerun_20210224 branch 2 times, most recently from b345b4c to 9f18f87 Compare July 14, 2021 20:17
@LouisDK1
Copy link
Contributor Author

@rycee

I find it strange that adding a single comment can result in a merge conflict. Can you see why?

SuperSandro2000
SuperSandro2000 reviewed Jul 15, 2021
View reviewed changes
pkgs/applications/video/hdhomerun-config-gui/default.nix Outdated
Comment on lines 1 to 3
{ lib, stdenv, fetchurl, libhdhomerun, pkg-config, gtk2 }:

# Version of libhdhomerun need to match the version of this package for successful build
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{ lib, stdenv, fetchurl, libhdhomerun, pkg-config, gtk2 }:
# Version of libhdhomerun need to match the version of this package for successful build
{ lib, stdenv, fetchurl, libhdhomerun, pkg-config, gtk2 }:

@SuperSandro2000
Copy link
Member

I find it strange that adding a single comment can result in a merge conflict. Can you see why?

just rebase it and resolve the merge conflict.

@stale
Copy link

stale bot commented Apr 25, 2022

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 Apr 25, 2022
@LouisDK1 @rycee
hdhomerun-config-gui: fix install bin path
5016b92 
Also make sure to run pre and post install hooks.
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 25, 2022
@rycee rycee merged commit 8540876 into NixOS:master Apr 25, 2022
@rycee
Copy link
Member

rycee commented Apr 25, 2022

Rebased, applied the suggestion of @SuperSandro2000, and merged to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@LouisDK1 @SuperSandro2000 @nixos-discourse @rycee