-
-
Notifications
You must be signed in to change notification settings - Fork 14.8k
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
system76-power: 1.2.1 -> 1.2.2 #350954
system76-power: 1.2.1 -> 1.2.2 #350954
Conversation
LGTM I will try to give it a test later today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it feasible to have upstream add a checksum to the sysfs-class
dependency? Then we could drop this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the Cargo.lock file is for any Rust project?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, if the Cargo.lock is inside the src tarball that is pulled then you just need to specify the hash of the file. See documentation and the previous version of this package:
cargoHash = "sha256-Vps02ZRVmeOQ8jDFZJYAUb502MhqY+2YV2W1/9XGY+0="; |
However, I am actually not sure which method is actually preferred between what we do now vs the cargoHash field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure either, this is how it is for the COSMIC packages that I have seen and maintain so it seems fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upstream is open to it, I can make a PR or you can. Just let me know and I'll get it handled either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure either, this is how it is for the COSMIC packages that I have seen and maintain so it seems fine.
It is allowed, but definitely disfavoured.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I had the steps to add it to upstream I'd be happy to do that and make another PR to remove it from this package!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new release of sysfs-class probably needs to be published to crates.io, and then system76-power upstream needs to use that released version rather than a git dependency, and then Nixpkgs needs to be updated to that version of system76-power.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One tiny thing. Otherwise looks fine.
12c6068
to
e4ffba5
Compare
What this does:
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.