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

Jump to bottom

qmidiarp: init at 0.6.5 #82795

Merged
merged 1 commit into from Mar 27, 2020
Merged

qmidiarp: init at 0.6.5 #82795

merged 1 commit into from Mar 27, 2020

Conversation

sjfloat
Copy link
Contributor

@sjfloat sjfloat commented Mar 17, 2020

Motivation for this change

Introduction of qmidiarp app.

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" New pkg, no dependents.
  • 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.

symphorien
symphorien requested changes Mar 17, 2020
View reviewed changes
Copy link
Member

@symphorien symphorien left a comment

Choose a reason for hiding this comment

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

Thanks for your PR. I left some comments to improve it a bit.

pkgs/applications/audio/qmidiarp/default.nix Show resolved Hide resolved
pkgs/applications/audio/qmidiarp/default.nix Outdated Show resolved Hide resolved
@sjfloat
Copy link
Contributor Author

sjfloat commented Mar 17, 2020

Do the checks reveal that I need to correct the sha256? It builds for me OK locally, but I'm not sure it aligns with the rev being used. Should it?

@symphorien
Copy link
Member

Try to replace the hash to all zeroes, and then rebuild. You should get the same good hash than in the error message.

@sjfloat
Copy link
Contributor Author

sjfloat commented Mar 17, 2020 •
edited

Try to replace the hash to all zeroes, and then rebuild. You should get the same good hash than in the error message.

Nice tip. nix-prefetch-git accepts a --rev argument, so I think I got the right one now. Note that I'm not getting an error message with either one building locally. But one of the @ofborg tests gave errors (though it passed everthing); this is what brought it to my attention.

@symphorien
Copy link
Member

Yes, if you have downloaded something (possibly unrelated) with this hash in the past, nix won't even try to download it again, even if you change the url.

@sjfloat
Copy link
Contributor Author

sjfloat commented Mar 17, 2020

BTW, I'm still not able to request reviews -- it's greyed out. I could ping @jonringer or @jtojnar, but I don't want to overburden them. Is there a more general way?

@symphorien
Copy link
Member

Huh I don't really know the inner workings or github...

@jtojnar
Copy link
Contributor

jtojnar commented Mar 17, 2020

Do the checks reveal that I need to correct the sha256? It builds for me OK locally, but I'm not sure it aligns with the rev being used. Should it?

Using nix-build -A foo.src --check might also work.

@sjfloat
Copy link
Contributor Author

sjfloat commented Mar 17, 2020

Do the checks reveal that I need to correct the sha256? It builds for me OK locally, but I'm not sure it aligns with the rev being used. Should it?

Using nix-build -A foo.src --check might also work.

Nice. I need to remember that!

I'll push up with the corrected hash.

@sjfloat
Copy link
Contributor Author

sjfloat commented Mar 17, 2020

I checked the data files produced by the app before and after adjusting the hash. It was indeed pulling from master. It's correct now.

symphorien
symphorien approved these changes Mar 17, 2020
View reviewed changes
Copy link
Member

@symphorien symphorien left a comment

Choose a reason for hiding this comment

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

  • Compiles locally
  • Runs fine, even in an empty PATH

@jonringer
Copy link
Contributor

Nice. I need to remember that!

You can also just put in an invalid sha (I usually just change the last character to a).

This may seem like a hack (it is), but some fetch helpers such as fetchpatch will do some processing on the contents and change the contents before determining the sha, and if you do a nix-prefetch, you'll get cached hits (the sha before fetchpatch does its processing).

jonringer
jonringer approved these changes Mar 17, 2020
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

diff LGTM
shows usage

[3 built, 1 copied (1.1 MiB), 0.2 MiB DL]
https://github.com/NixOS/nixpkgs/pull/82795
1 package built:
qmidiarp

@jonringer
Copy link
Contributor

@GrahamcOfBorg build qmidiarp

@sjfloat
Copy link
Contributor Author

sjfloat commented Mar 20, 2020

@jonringer is there anything holding this up at this point?

@jonringer
Copy link
Contributor

no, I was just busy, sorry

@jonringer jonringer merged commit 28aa914 into NixOS:master Mar 27, 2020
@sjfloat
Copy link
Contributor Author

sjfloat commented Mar 27, 2020

no, I was just busy, sorry

No apologies! You're doing a great job!

Thanks

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

@jonringer jonringer jonringer approved these changes

@symphorien symphorien symphorien approved these changes

@jtojnar jtojnar Awaiting requested review from jtojnar

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@sjfloat @symphorien @jtojnar @jonringer