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.

Sign up for GitHub

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

protonmail-bridge: 1.1.5 -> 1.1.6 #65315

Merged
merged 1 commit into from Jul 24, 2019
Merged

protonmail-bridge: 1.1.5 -> 1.1.6 #65315

merged 1 commit into from Jul 24, 2019

Conversation

GRBurst
Copy link
Contributor

@GRBurst GRBurst commented Jul 23, 2019 •
edited

Motivation for this change

Update: 1.1.5 -> 1.1.6

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.

@GRBurst GRBurst changed the title protonmail-bridge: 0.1.5 -> 0.1.6 protonmail-bridge: 1.1.5 -> 1.1.6 Jul 23, 2019
@mmahut
Copy link
Member

mmahut commented Jul 23, 2019

@GrahamcOfBorg build protonmail-bridge

mmahut
mmahut approved these changes Jul 23, 2019
View reviewed changes
Copy link
Member

@mmahut mmahut left a comment

Choose a reason for hiding this comment

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

  • reviewed the diff and commit messages
  • made sure ofBorg build succeeded for all applicable platforms
  • run nix-review without any failures
  • run and tested the binaries

@worldofpeace
Copy link
Contributor

This comment

Also you should switch this expression to use mkDerivation which will automatically add wrapQtAppsHook.
to nativeBuildInputs without explicitly specifying this

still stands.

@GRBurst
Copy link
Contributor Author

GRBurst commented Jul 24, 2019 •
edited

Also you should switch this expression to use mkDerivation which will automatically add wrapQtAppsHook.
to nativeBuildInputs without explicitly specifying this

I don't get that exactly. The package is build with a mkDerivation expression (line 24). What am I missing?

@GRBurst
Copy link
Contributor Author

GRBurst commented Jul 24, 2019

Also you should switch this expression to use mkDerivation which will automatically add wrapQtAppsHook.
to nativeBuildInputs without explicitly specifying this

I don't get that exactly. The package is build with a mkDerivation expression (line 24). What am I missing?

Oh - I guess I misunderstood your comment.

@worldofpeace
Copy link
Contributor

libsForQt5.callPackage brings is qt5 in scope so it would be qt5.mkDerivation.

so stdenv.mkDerivation != mkDerivation in this case.

Understand the confusion, it makes the expression appear rather ambiguous.

@GRBurst
Copy link
Contributor Author

GRBurst commented Jul 24, 2019

libsForQt5.callPackage brings is qt5 in scope so it would be qt5.mkDerivation.

so stdenv.mkDerivation != mkDerivation in this case.

Understand the confusion, it makes the expression appear rather ambiguous.

Thank you for your guidance 👍

@worldofpeace
Copy link
Contributor

But libsForQt5.callPackage brings qt5.mkDerivation in scope so you must reference it as just mkDerivation.

Same with qtbase etc.

@GRBurst
Copy link
Contributor Author

GRBurst commented Jul 24, 2019 •
edited

If I reference it as just mkDerivation this does not work.

error: undefined variable 'mkDerivation' at /nixpkgs/pkgs/applications/networking/protonmail-bridge/default.nix:24:4

Hence I added qt5 such that I can call qt5.mkDerivation, which worked (compiles and binary runs).

@worldofpeace
Copy link
Contributor

You'd need to add it to the function arguments

{ ... , mkDerivation }:

worldofpeace
worldofpeace approved these changes Jul 24, 2019
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

Builds and executes for me locally.

@worldofpeace worldofpeace merged commit 2bcc926 into NixOS:master Jul 24, 2019
@worldofpeace
Copy link
Contributor

After merging this I noticed that the archive includes a desktop file that's more complete than the one we generate.

[Desktop Entry]
Name=ProtonMail Bridge
GenericName=ProtonMail Bridge for Linux
Comment=The Bridge is an application that runs on your computer in the background and seamlessly encrypts and decrypts your mail as it enters and leaves your computer.
Exec=protonmail-bridge
Icon=/usr/share/icons/protonmail/ProtonMail_Bridge.svg
Type=Application
Version=1.0
Terminal=false
Categories=Utility;Security;Network;Email

Though those paths would need to be patched with substitute.

@GRBurst GRBurst mentioned this pull request Sep 8, 2019
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmahut mmahut mmahut approved these changes

@worldofpeace worldofpeace worldofpeace approved these changes

Assignees
No one assigned
Labels
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
3 participants
@GRBurst @mmahut @worldofpeace