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

Jump to bottom

yubikey-manager-qt: build using Qt mkDerivation #67613

Closed
wants to merge 1 commit into from
Closed

yubikey-manager-qt: build using Qt mkDerivation #67613

wants to merge 1 commit into from

Conversation

peterhoeg
Copy link
Member

Motivation for this change

Further to #65399

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-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
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @avdv

@worldofpeace
Copy link
Contributor

I'm intrigued by where the python code is in this application.
A search on the repo reveals this

However I don't see those files in the output

result
├── bin
│   └── ykman-gui
└── share
    ├── applications
    │   └── ykman-gui.desktop
    └── ykman-gui
        └── icons
            ├── ykman.icns
            ├── ykman.ico
            └── ykman.png

5 directories, 5 files

So I think ykman-cli uses python, and perhaps python is used during the build?

@avdv
Copy link
Member

avdv commented Aug 28, 2019

I'm intrigued by where the python code is in this application.

I think it gets packaged into the binary as a resource. Depending on how the app is started (out of the build directory or not) it gets loaded via the resource or directly from the file system: https://github.com/Yubico/yubikey-manager-qt/blob/master/ykman-gui/main.cpp#L58-L70

avdv
avdv reviewed Aug 28, 2019
View reviewed changes
pkgs/tools/misc/yubikey-manager-qt/default.nix
postPatch = ''
substituteInPlace ykman-gui/deployment.pri --replace '/usr/bin' "$out/bin"
substituteInPlace ykman-gui/deployment.pri \
--replace /usr/bin $out/bin
Copy link
Member

Choose a reason for hiding this comment

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

Why did you change this line? Did you ran some linter tool? But then, why didn't you also change a similar line for the substituteInPlace call at line 54?

Usually, I am in favor of proper quoting, to be safe. But at least it should be consistent in the same file.

avdv
avdv reviewed Aug 28, 2019
View reviewed changes
Copy link
Member

@avdv avdv left a comment

Choose a reason for hiding this comment

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

LGTM, I build it locally and it worked for me.

@worldofpeace
Copy link
Contributor

I'm intrigued by where the python code is in this application.

I think it gets packaged into the binary as a resource. Depending on how the app is started (out of the build directory or not) it gets loaded via the resource or directly from the file system: https://github.com/Yubico/yubikey-manager-qt/blob/master/ykman-gui/main.cpp#L58-L70

Yes it does generate a resources.qrc during the build with that python script as an input.
Though I'm wondering this, does the shebang get corrected before it gets embedded?
If not that would introduce an impurity relying on python being in the environment.

@avdv
Copy link
Member

avdv commented Aug 28, 2019

Though I'm wondering this, does the shebang get corrected before it gets embedded?
If not that would introduce an impurity relying on python being in the environment.

I don't think the shebang is in any way relevant, since the file is never unpacked on disc for a shell to consume. It is directly called by the embedded interpreter.

@worldofpeace
Copy link
Contributor

Though I'm wondering this, does the shebang get corrected before it gets embedded?
If not that would introduce an impurity relying on python being in the environment.

I don't think the shebang is in any way relevant, since the file is never unpacked on disc for a shell to consume. It is directly called by the embedded interpreter.

Ahh, I guess it's used as a module

worldofpeace
worldofpeace reviewed Aug 28, 2019
View reviewed changes
pkgs/tools/misc/yubikey-manager-qt/default.nix
buildInputs = [ pythonPackages.python qtbase qtgraphicaleffects qtquickcontrols qtquickcontrols2 pyotherside ];
buildInputs = [ python3Packages.python qtbase qtgraphicaleffects qtquickcontrols qtquickcontrols2 pyotherside ];

nativeBuildInputs = [ python3Packages.wrapPython qmake ];
Copy link
Contributor

Choose a reason for hiding this comment

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

wrapPython isn't going to do anything to the files in this package.
You could however get it to apply its sed expression to the ykman-cli.py file, and then I think you shouldn't need the PYTHONPATH wrapper.

@stale
Copy link

stale bot commented Jun 1, 2020

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the
    related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
    irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020
@peterhoeg peterhoeg self-assigned this Jun 1, 2020
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 1, 2020
@stale
Copy link

stale bot commented Nov 28, 2020

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 28, 2020
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 18, 2021
@stale
Copy link

stale bot commented Jul 19, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 19, 2021
@peterhoeg
Copy link
Member Author

Someone already did this. No longer needed.

@peterhoeg peterhoeg closed this Mar 23, 2022
@peterhoeg peterhoeg deleted the f/ykman-qt branch March 23, 2022 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@avdv avdv avdv left review comments

@worldofpeace worldofpeace worldofpeace left review comments

Assignees

@peterhoeg peterhoeg

Labels
2.status: merge conflict 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@peterhoeg @worldofpeace @avdv @SuperSandro2000