-
-
Notifications
You must be signed in to change notification settings - Fork 14.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
criterion: init at version 2.3.3 #70609
Conversation
I hope this is not a duplicate to #44911 |
Also, while reading through #44911 I found out that the unstable versioning scheme should be |
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 entirely unsure why I was specifically mentioned on this PR... but while I'm here, a few things I noticed 😄
nanomsg | ||
]; | ||
|
||
# CMake needs a little help to find BoxFort and pkg-config |
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 this a problem with BoxFort
? If a fix for this can be applied in BoxFort
instead of here that would be more desirable as every package/developer on nix using BoxFort
conceivably has to implement this workaround.
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.
Seeing as pkg-config
goes undetected the same way, I don't think it's a boxfort
problem.
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.
So, is it resolved or no?
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've seen i where some people will just see if a directory exists, instead of using the find_packge
command. I think this is alright, but it would probably be good to open a ticket upstream
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.
Will do. I'm assuming all reqs are cleared and we can merge, right? It still says "changes requested"
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 notice the other PR didn't have need to add this .third-party
directory.
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 suspect their modification of $LD_LIBRARY_PATH
fixed it on their side, I tested it locally and it works
@Thesola10 Ahh ok. Absolutely no problem at all! Feel free to ping me any time 😄 I was just unsure and thought maybe I should know something about this PR that I didn't. I'm always happy to help if I'm able and feel flattered you'd mention me 👍 |
By the way, are we good to merge? It still says “Changes requested”. |
Also I recently found out about #70403 and I'm wondering if my PR is a dupe. If that one's more likely to be merged I don't mind giving up this PR. |
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.
@Thesola10 You have accidentally added 2 commits to this PR from someone else.
I'd ask that you and @Yumasi decide which PR to go forward with, add both of you as maintainers, and then close the other PR and then we can proceed.
Thanks for your hard work so far! We're almost there.
Motivation for this change
There is currently no true distro-agnostic way to install the
criterion
library on Linux, and I thought Nix was a good choice.Note:
boxfort
does not have official tagging/versioning, so I set the version attribute to be the commit hash (like GitHub does).Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
) (does not apply, but generated libraries work properly.)nix path-info -S
before and after)Notify maintainers
cc @aanderse