-
-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
libimobiledevice: 1.3.0-date=2023-04-30 -> 1.3.0-unstable-2024-05-20, libimobiledevice-glue: 1.2.0 -> 1.3.0 #320999
Conversation
You have to split in different commits. (And follow the contribution guidelines.) |
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.
Please split the two package updates into their own commit and properly format commits and PR title to something like libimobiledevice: 1.3.0-date=2023-04-30 -> 1.3.0-date=2024-06-19
and libimobiledevice-glue: 1.2.0 -> 1.3.0
.
https://github.com/NixOS/nixpkgs/blob/master/pkgs/README.md#package-naming Is this I think it should be |
I'm not sure, this package has been using it since before I became its maintainer. We always could change it or just keep the flow. |
Thanks for clarifying. I think we should follow the guidelines for contribution unless there is a good reason. And then, the good reason should be documented. I think, this may be just the usual case of |
This package is missing the update script. There is a Please add:
Note: For testing:
|
2f91e60
to
d52536b
Compare
I've removed the Please note, that the previous build still has not completed for me. I take it, that's all you wanted to happen? |
A single PR can have multiple commits. The problem was that you added more than a single package to a single commit.
Good. That's how it's supposed to be. Always favor the update script over manual updates. Only in cases of failure, that the package or update script have to be tweaked that you should do manual updates.
FYI, eval is different than building a package. If you propose a package that you haven't built, and you don't know it is building, you may break it for everyone. (And no one wants that.) And you also have to build the downstream of it using
:-) |
There is a certain format to commit messages. There exist automation to do auto sort of things to get your PR right. Follow this format:
Note: I only want to point out you are missing the |
2494e11
to
06b73e6
Compare
Up to 500, target master and OfBorg can handle it. |
I think, you need to specify the packages that you want to be built. If there is downstream packages that you want to be built, you have to specify it too. Instructions at: https://github.com/NixOS/ofborg?tab=readme-ov-file#ofborg |
I think I'm just not a trusted user. ofborg build only takes in a list of parameters to be passed to nix-build iirc. |
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.
Thank you @superherointj for reviewing. I'm at work right now but I'll take a look this evening when I'm free.
My host failed to build it. Indeed, this build is large. I think, you should target staging. Despite not being that many packages numbers wise... Mark it as a draft first. Because odds are you'll get this wrong and ping everyone in nixpkgs. (I'm saying this because people often get this wrong. Not you specifically.) |
Set as draft, rebase, retarget, and unmark as draft @C0D3-M4513R |
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.
Rebase to staging.
First, mark the PR as draft. (Actually, only verify it is marked as draft. I've marked it as draft for you.)
With instructions as clear as those in the contributing document, it was pretty straight forward. (thanks for linking) |
The trick to do larger builds is to lower My host is larger than yours and I still failed to build. I think, it is better to leave this for hydra at this point. Certify to look up packages and logs at Hydra when build is done: https://hydra.nixos.org/project/nixpkgs I'll be merging this PR when eval is done. |
Did I rebase to the right branch? Hydra reports the last staging build to be at |
Hydra builds the other branch (staging-next). (But you did name your branch with an unfortunate/misleading name (master), instead of |
The naming isn't the best, but I'm scared to try to rename the branch using github's ui. Also I suppose this supersedes #320292 . |
I did not suggest you to rename it at this point in time. Only mentioned it for you awareness. (That you pick a better name for next time.)
Yes, will close it. |
Something could be off. Or staging/staging-next has been merged. [Likely this.] To guarantee it is solid. Let's do the following: I have named
Result will be like this: [minus If you do this, you'll have a clean staging with your commits for sure. Then, you have to force push it to the remote branch you have used. |
Signed-off-by: C0D3 M4513R <[email protected]>
Signed-off-by: C0D3 M4513R <[email protected]>
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.
Tested eval on my host. I expect this to be fine.
I'm merging without waiting for CI eval because it takes forever. And I just tested eval (not full build, can't do full build).
The build regressed on |
Description of changes
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.