Skip to content
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

Merged
merged 0 commits into from Nov 12, 2020
Merged

otpgen: init at 97c7aa #87887

merged 0 commits into from Nov 12, 2020

Conversation

SCOTT-HAMILTON
Copy link
Contributor

@SCOTT-HAMILTON SCOTT-HAMILTON commented May 15, 2020

Motivation for this change

OTPGen wasn't packaged yet.

Things done

Packaged OTPGen GUI and CLI + migration tools.

  • Tested using sandboxing
  • Built on platform(s)
    • [X ] NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • [X - normally] Fits CONTRIBUTING.md.

pkgs/tools/security/otpgen/default.nix Outdated Show resolved Hide resolved
pkgs/tools/security/otpgen/default.nix Outdated Show resolved Hide resolved
pkgs/tools/security/otpgen/default.nix Outdated Show resolved Hide resolved
pkgs/tools/security/otpgen/default.nix Outdated Show resolved Hide resolved
pkgs/tools/security/otpgen/default.nix Outdated Show resolved Hide resolved
@SCOTT-HAMILTON
Copy link
Contributor Author

Fixed build error with ${version} (forgot to escape those sequences, bash interpeted it and it didn't get in the commit comment)

@SCOTT-HAMILTON
Copy link
Contributor Author

Checks should be fixed when merging my addition to the maintainers list : #87938

@bbjubjub2494
Copy link
Member

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.

@SCOTT-HAMILTON
Copy link
Contributor Author

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.

@bbjubjub2494
Copy link
Member

No worries.

Yes, that should help OfBorg.

Yes go ahead and close it.

@SCOTT-HAMILTON
Copy link
Contributor Author

Patches have been merged upstream https://github.com/magiruuvelvet/OTPGen/

@jtojnar
Copy link
Contributor

jtojnar commented May 17, 2020

@jtojnar
Copy link
Contributor

jtojnar commented May 17, 2020

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

Copy link
Contributor

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some more nitpicking.

pkgs/tools/security/otpgen/default.nix Outdated Show resolved Hide resolved
pkgs/tools/security/otpgen/default.nix Outdated Show resolved Hide resolved
pkgs/tools/security/otpgen/default.nix Outdated Show resolved Hide resolved
pkgs/tools/security/otpgen/default.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/tools/security/otpgen/default.nix Outdated Show resolved Hide resolved
@SCOTT-HAMILTON
Copy link
Contributor Author

For the bundled libs, an issue has been opened, I'll try to fix it myself if nothing is done.

@SCOTT-HAMILTON
Copy link
Contributor Author

Issue is here

@wamserma
Copy link
Member

wamserma commented May 17, 2020

Cool. Then you can use fetchpatch with https://github.com/magiruuvelvet/OTPGen/commit/119dc46f1571c377c7eaf2d167deb142f6dcc9d6.patch and https://github.com/magiruuvelvet/OTPGen/commit/afd3fc87c4422bdf4f3978d4a2de1565a1d93283.patch instead of having the patches in-tree.

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

@SCOTT-HAMILTON
Copy link
Contributor Author

I already fixed it a few commits ago f70af1e

@SCOTT-HAMILTON
Copy link
Contributor Author

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.
There also are libs that aren't yet in nixpkgs such as https://github.com/glassechidna/zxing-cpp/.
For those, I'll see what I can do to package them.

@SCOTT-HAMILTON
Copy link
Contributor Author

Started a new PR for zxing-cpp dependency here #88065

@SCOTT-HAMILTON
Copy link
Contributor Author

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 :

-- Found ZLIB: /nix/store/kf2sh7c2fmizvycqq02aad1rfp6xswgv-zlib-1.2.11/lib/libz.so (found version "1.2.11")

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.

@SCOTT-HAMILTON
Copy link
Contributor Author

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.

@jtojnar
Copy link
Contributor

jtojnar commented May 18, 2020

prePatch is fine.

Regarding the zxing-cpp, a compromise would be to introduce it to nixpkgs but not to all-packages.nix, only put it to the otpgen directory. Example:

let librustzcash = callPackage ./librustzcash {};

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.

@jtojnar
Copy link
Contributor

jtojnar commented May 18, 2020

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.

@jtojnar
Copy link
Contributor

jtojnar commented May 18, 2020

Could you share the error message from the cereal library?

@SCOTT-HAMILTON
Copy link
Contributor Author

@SCOTT-HAMILTON
Copy link
Contributor Author

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.

@jtojnar
Copy link
Contributor

jtojnar commented May 18, 2020

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.)

@SCOTT-HAMILTON
Copy link
Contributor Author

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.
For me, I'm done trying to convert HEADs commit patches to older v0.9.3 compatible patches. I will wait for the 2.0 to come (and will help if needed). So this thread is in hibernation (if such a thing exists) until the software is ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants