-
-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
crumbs: init at version 0.0.3 #58960
Conversation
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.
Thanks for your PR!
You forgot to include the callPackage line in pkgs/top-level/all-package.nix
declaring crumbs. I also left a few comments to improve the derivation, but they are not so important.
I have submitted new changes applying your suggested changes. Tested the new derivation and it installs properly. |
This should ideally be rebased into one commit with the message |
I added a few more comments, but it looks good to me overall. |
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 rewrite into 2 commits: the first adding yourself to maintainer list, the second adding the package.
Removed a trailing comma in the includes header of |
@Thesola10 looks like you might have mixed something up on that last push:
|
@aanderse there's a reason behind it. I think I had used a specific commit for reference instead of version 0.0.2 |
@Thesola10 is this currently building on your system? I pulled again and got this:
|
this is very weird. noticed it also does that with |
Upon second look, it seems that the |
Also, I'm kind of weirded out by the fact that @GrahamcOfBorg doesn't seem to test whether the actual derivation can build properly... |
I'll be asking the bot to build your program once it builds on my machine.
Please rewrite the git history. |
Of course. I was asking if there was a better way than to redefine |
Ah... sorry I read that wrong the first time 😆 I'll defer to @jtojnar for all |
You really should not override Or you can override the flags
|
@jtojnar thanks for the advice! After testing, though, what worked was setting |
I would not expect It looks like they have pre-generated |
You're right, it does not seem necessary. In any case, using |
After another test, it would seem that configure is pre-generated. Which is weird, since I remember a previous version required to |
@Thesola10 seems like only outstanding issue is addressing the 0.0.3 comment mentioned in #58960 (comment). |
Do you mean that I need to bump the derivation to crumbs |
I think all of the discrepancies in the build process (like |
I did mean that. I ran checked out your branch and it does build fine as is on 0.0.3. |
Would adding it into |
Here are a few examples of bash and fish shell completions searching through the codebase turned up: |
Added the autocomplete scripts in a |
Looking good. The fish completions reference gfind instead of find so I made this change and then they worked:
and in the postInstall
|
Wouldn't the reference to |
|
The original repo mentions a |
As far as I can tell |
Side note -- why is the "changes requested" blocker still present? Is my branch ready for merging? |
@GrahamcOfBorg build crumbs I guess I was waiting to hear if you were satisfied with my response 😄 |
@Thesola10 thank you for your contribution! 🎉 |
Let me know if there are any errors with the way this derivation is written.
Motivation for this change
I wanted to add this little utility to Nix to make installing it easier.
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
(None depend anyway)./result/bin/
)nix path-info -S
before and after)