-
-
Notifications
You must be signed in to change notification settings - Fork 14.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
otpgen: init at 97c7aa #87887
otpgen: init at 97c7aa #87887
Conversation
Fixed build error with ${version} (forgot to escape those sequences, bash interpeted it and it didn't get in the commit comment) |
Checks should be fixed when merging my addition to the maintainers list : #87938 |
Does ofborg checkout the tip of the PR or the merge tho? If it's the former you're going to have to merge it here regardless. |
My english is a little bit rusted, are you asking me to merge the maintainers-list.nix modifications here? If so then I probably should close this now #87938. |
No worries. Yes, that should help OfBorg. Yes go ahead and close it. |
Patches have been merged upstream https://github.com/magiruuvelvet/OTPGen/ |
Cool. Then you can use |
At this point, the package looks quite good but I am quite concerned about the number of bundled dependencies: https://github.com/magiruuvelvet/OTPGen/tree/master/Libs |
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.
some more nitpicking.
For the bundled libs, an issue has been opened, I'll try to fix it myself if nothing is done. |
Issue is here |
I already suggested that on the initial version of the PR. Me reviews somehow got lost with all the updates.. ah, here it was: 9c7dcca#r426058136 :D |
I already fixed it a few commits ago f70af1e |
I failed moving to system cereal library. The author pointed out that some libs were modified so maybe this is the reason. He also pointed out that the boost stuff could be moved to c++17 goodies. |
Started a new PR for zxing-cpp dependency here #88065 |
Testing this snippet to default.nix : preConfigurePhases = [ "prePatch" ];
prePatch = ''
rm -rf Libs/zlib
''; I get no error building, seems like cmake successfully found system's zlib :
This should confirm that we can successfully remove the bundled zlib. I'm just not sure if the prePatch thing is the way to go. |
The zxing-cpp lib is not really suitable for being added to nixpkgs as explained here #88065 (comment). So our best choice is to keep the bundled zxing-cpp for the moment. |
Regarding the
Of course, it would be best if the project did not use unmaintained libraries. I would suggest https://github.com/mchehab/zbar as an alternative that is maintained and has a Qt widget. |
Also there is no need to open new pull request when the packages are only needed for optgen (unless the changes are super big or something). We can just review it here. |
Could you share the error message from the cereal library? |
Here is the error logs : https://gist.github.com/SCOTT-HAMILTON/bd5015b9bef7a9738b830d2651057169 |
I have also managed to replace boost with std::filesystem but the patches aren't compatible with latest commit. So either I patch the v0.9.3 tag's commit or I put the patch in nixpkgs repo alongside default.nix. And of course I'll make a new patch to fit upstream HEAD. |
Yeah, in that case, it will have to be in the nixpkgs repo. (Unless the project decides to create a 0.9 maintenance branch against which the 0.9.3 patches could be filed.) |
So magiruuvelvet (OTPGen's author) is planning on rewriting it for a fresh new 2.0, no more modified libs, less dependencies, zbar instead of the unsupported zxing-cpp and other things. You can follow the proposal here magiruuvelvet/OTPGen#7. |
ba76e48
to
cc8a3b4
Compare
Motivation for this change
OTPGen wasn't packaged yet.
Things done
Packaged OTPGen GUI and CLI + migration tools.
nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)