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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

tpm2-openssl: init at 1.2.0 #299626

Merged
merged 1 commit into from
Apr 8, 2024
Merged

Conversation

stv0g
Copy link
Contributor

@stv0g stv0g commented Mar 28, 2024

Description of changes

This PR packages tpm2-openssl an OpenSSL Provider for TPM2 integration.

Tested with:

nix build github:stv0g/nixpkgs/add-tpm2-openssl\#tpm2-openssl
nix run   github:stv0g/nixpkgs/add-tpm2-openssl\#openssl -- rand \
   -provider-path ./result/lib/ossl-modules \
   -provider tpm2 \
   16

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 馃憤 reaction to pull requests you find important.

Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

I tested it out and it does indeed work as expected.

I have a few suggestions and comments.

pkgs/by-name/tp/tpm2-openssl/package.nix Show resolved Hide resolved
pkgs/by-name/tp/tpm2-openssl/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/tp/tpm2-openssl/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/tp/tpm2-openssl/package.nix Outdated Show resolved Hide resolved
Copy link
Contributor

@thillux thillux left a comment

Choose a reason for hiding this comment

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

Please use a recursive attribute set instead of finalAttrs and reformat afterwards. See the attached patch for reference. Otherwise LGTM.

diff --git a/pkgs/by-name/tp/tpm2-openssl/package.nix b/pkgs/by-name/tp/tpm2-openssl/package.nix
index a3cce7058653..1a0d6454a3e2 100644
--- a/pkgs/by-name/tp/tpm2-openssl/package.nix
+++ b/pkgs/by-name/tp/tpm2-openssl/package.nix
@@ -6,16 +6,16 @@
 , pkg-config
 , openssl
 , tpm2-tss
-,
 }:
-stdenv.mkDerivation (
-  finalAttrs: {
+
+stdenv.mkDerivation rec {
     pname = "tpm2-openssl";
     version = "1.2.0";
+
     src = fetchFromGitHub {
       owner = "tpm2-software";
       repo = "tpm2-openssl";
-      rev = finalAttrs.version;
+      rev = version;
       hash = "sha256-mZ4Z/GxJFwwfyFd1SAiVlQqOjkFSzsZePeuEZtq8Mcg=";
     };
 
@@ -37,7 +37,7 @@ stdenv.mkDerivation (
     '';
 
     prePatch = ''
-      echo ${finalAttrs.version} > VERSION
+      echo ${version} > VERSION
     '';
 
     meta = with lib; {
@@ -47,5 +47,4 @@ stdenv.mkDerivation (
       maintainers = with maintainers; [ stv0g ];
       platforms = platforms.linux;
     };
-  }
-)
+}

@stv0g
Copy link
Contributor Author

stv0g commented Apr 3, 2024

Please use a recursive attribute set instead of finalAttrs and reformat afterwards. See the attached patch for reference. Otherwise LGTM.

Thanks for the review :) I am wondering, why is using finalAttrs discouraged here? In case somebody overrides the package version, wouldnt this change be missed in the VERSION file?

@philiptaron
Copy link
Contributor

#119942 and #293452 and this tip on nix.dev suggest we're broadly abandoning rec, especially in this use case. I don't understand your rationale for recommending the other direction @thillux -- please say more.

@thillux
Copy link
Contributor

thillux commented Apr 3, 2024

#119942 and #293452 and this tip on nix.dev suggest we're broadly abandoning rec, especially in this use case. I don't understand your rationale for recommending the other direction @thillux -- please say more.

Fair, I informed myself about this (thanks @WilliButz). Never mind my comments :)

@0x4A6F
Copy link
Member

0x4A6F commented Apr 4, 2024

Result of nixpkgs-review pr 299626 run on x86_64-linux 1

1 package failed to build:
  • tpm2-openssl

@stv0g
Copy link
Contributor Author

stv0g commented Apr 7, 2024

Okay, I found the issue. It was broken by this recommendation by @SuperSandro2000
#299626 (comment).

Environment variables are not substituted in configureFlags. Or only if properly escaped:

---  configureFlags = [ "--with-modulesdir=$out/lib/ossl-modules" ];
+++  configureFlags = [ "--with-modulesdir=$$out/lib/ossl-modules" ];

I've seen that this is handled quite inconsistently in Nixpkgs. Some packages are using the escaping with $$, others are extending configureFlags in the preConfigure hook.

I will draft another PR to align this tree-wide.

@SuperSandro2000
Copy link
Member

I think you could also use $(out)

@SuperSandro2000 SuperSandro2000 merged commit fd62257 into NixOS:master Apr 8, 2024
23 of 24 checks passed
@stv0g stv0g deleted the add-tpm2-openssl branch April 8, 2024 09:00
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