implement bundled feature and improve windows support #523

uglyoldbob · 2024-04-15T14:58:17Z

This pull request adds a feature bundled. It builds the tpm2-tss library from source after fetching with git.
The normal requirements for build the software apply.

The linux build needs all of the normal requirements for building tpm2-tss as described in their repository.

Windows builds require visual studio 2017, with the c2 clang option, visual studio build tools 10.0.17134.0. Openssl should be installed to C:\OpenSSL-v11-Win64

tss-esapi-sys/build.rs

Superhepper · 2024-04-16T14:57:05Z

Excellent work. I will have a look at those errors when I get the time to see if we can get this merged.

uglyoldbob · 2024-04-16T16:38:24Z

I have a few changes required for bundled on linux remaining.

tss-esapi-sys/build.rs

Superhepper · 2024-04-19T14:54:11Z

Only thing left for you to do is signing the commits.

uglyoldbob · 2024-04-29T00:33:58Z

The latest commit on the pull request should have signature on it now.

Firstyear · 2024-05-01T05:15:21Z

tss-esapi-sys/Cargo.toml

 [features]
 generate-bindings = ["bindgen"]
+bundled = ["dep:autotools", "dep:msbuild", "dep:winreg"]


Wouldn't bundled need to imply bindgen too? 🤔

I had thought that, but on my Ubuntu installation, bindgen didn't work. I had to do bundled without bindgen there.
On windows I needed bindgen and bundled.

No bundled does need to imply bindgen. Bundled can use the bindings that are committed in the sys crate and just build the libraries from the specified version.

ionut-arm

Thanks for the patch!

ionut-arm · 2024-05-03T10:47:31Z

tss-esapi-sys/Cargo.toml

 pkg-config = "0.3.18"
 target-lexicon = "0.12.0"
 cfg-if = "1.0.0"
 semver = "1.0.7"

+[target.'cfg(windows)'.build-dependencies]
+msbuild = { git = "https://github.com/uglyoldbob/msbuild.git", optional = true }


I'd strongly prefer if this was published to crates.io and we included it that way. Otherwise I'm not even sure we can publish tss-esapi-sys.

Oh right I forgot about that. I'll check to make sure I published all my code for that crate.

ionut-arm · 2024-05-03T10:55:29Z

tss-esapi-sys/Cargo.toml

 [features]
 generate-bindings = ["bindgen"]
+bundled = ["dep:autotools", "dep:msbuild", "dep:winreg"]


Could you please add some text in the README file and/or in the code documentation to describe the use of the new feature?

Sure I can add some documentation.

ionut-arm · 2024-05-03T11:37:19Z

tss-esapi-sys/build.rs

+        pub fn bundled() -> Self {
+            use std::io::Write;
+            let out_path = std::env::var("OUT_DIR").expect("No output directory given");
+            let source_path = Self::fetch_source(


Worth making it explicit in documentation about how this behaves in relationship with the build environment, i.e. that if you don't have a local copy of tpm2-tss then you need access to the internet.

Sure, that makes sense.

ionut-arm · 2024-05-03T11:44:06Z

tss-esapi-sys/build.rs

+                source_path.pop();
+                source_path.pop();
+                source_path.pop();
+                println!("Source path is {}", source_path.display());


Was this here on purpose, or just a debugging remnant?

Just a debugging remnant that was accidentally left in.

ionut-arm · 2024-05-03T11:44:51Z

tss-esapi-sys/build.rs

+                let build_string = match profile.as_str() {
+                    "debug" => "Debug",
+                    "release" => "Release",
+                    _ => panic!("Unknown cargo profile:"),


Does this actually print what the profile string was after the ":"?

Not sure, will need to test it.

ionut-arm · 2024-05-03T22:55:30Z

tss-esapi-sys/build.rs

+                    let hklm = winreg::RegKey::predef(winreg::enums::HKEY_LOCAL_MACHINE);
+                    let sdk_entry = hklm.open_subkey("SOFTWARE\\WOW6432Node\\Microsoft\\Microsoft SDKs\\Windows\\v10.0").unwrap();
+                    let installation_path: String = sdk_entry.get_value("InstallationFolder").unwrap();
+                    let ip_pb = PathBuf::from(installation_path).join("Include");
+                    let windows_sdk = ip_pb.join("10.0.17134.0");
+                    clang_args.push(format!("-I{}", windows_sdk.join("ucrt").display()));
+                    clang_args.push(format!("-I{}", windows_sdk.join("um").display()));
+                    clang_args.push(format!("-I{}", windows_sdk.join("shared").display()));
+                    Some(clang_args)


Any chance there's some documentation somewhere as to what this is doing? Not in this crate, just generally, seems like a lot of hardcoded things.

It finds the windows sdk (specifically version 10.0.17134.0) from the registry, and adds the relevant paths for clang.

The only thing that stands out to me is the version of the SDK. Is it possible to have something less specific? Like "latest" or "10.0"?

Firstyear · 2024-07-28T04:10:55Z

@ionut-arm @Superhepper Should we re-review this? This will assist me with MacOS development.

wiktor-k · 2024-07-29T07:43:56Z

@ionut-arm @Superhepper Should we re-review this? This will assist me with MacOS development.

Just FYI I'm not opposed to merging this but I've been unable to successfully test it in a reasonable time, both on a Windows machine locally and via CI (#524).

Firstyear · 2024-07-30T01:18:19Z

I've been trying to test it on macos, but there are issue in the tpm2-tss side, I need to double check it on linux.

Firstyear · 2024-07-30T06:41:22Z

@ionut-arm @Superhepper Should we re-review this? This will assist me with MacOS development.

Just FYI I'm not opposed to merging this but I've been unable to successfully test it in a reasonable time, both on a Windows machine locally and via CI (#524).

#531

My branch on top of this tests on linux and has it working :)

Superhepper

I would have liked for the SDK version to be more flexible but I am ok this as it is now.

ionut-arm

I'm happy with the changes as well, what do we do about the CI?

Superhepper · 2024-08-05T07:54:22Z

This needs to be rebased on the latest main.

Superhepper · 2024-08-17T07:27:27Z

@uglyoldbob It would be greate if you could rebase this on lastest from main so we could get it merged.

uglyoldbob · 2024-08-17T13:02:21Z

@Superhepper Ok I applied my changes to the latest on main, even with gpg signatures on the two commits. I couldn't figure out how to squash them into a single commit.

Superhepper · 2024-08-18T18:36:43Z

@Superhepper Ok I applied my changes to the latest on main, even with gpg signatures on the two commits. I couldn't figure out how to squash them into a single commit.

I usually squash commits using interactive rebase and remove all the signing thing from the comment. Then I sign the commit using amend.

wiktor-k · 2024-08-19T09:11:30Z

Ok I applied my changes to the latest on main, even with gpg signatures on the two commits. I couldn't figure out how to squash them into a single commit.

Just for the record when you do git rebase main (or something like that) you can also do git rebase -i main to start "interactive" rebase which would present you with a list of commits. If you mark the later ones as squash or fixup they'd be melded with previous ones automatically. Just FYI. 👋

When parallaxsecond#477 got merged it became possible to build using a path to the ```tpm2-tss``` installation instead of depending on ```pkg-config```. This made it possible to build under Windows. To further increase the support for the windows platform this commit moves the option for TBS TCTI that is being introduced in parallaxsecond#523 into a separate commit. This commit also updates the documentation regarding building using an installation folder. Co-author: Thomas Thomas Epperson <thomas.epperson@gmail.com> Signed-off-by: Jesper Brynolf <jesper.brynolf@gmail.com>

Superhepper · 2024-09-13T20:57:14Z

I saw that there were some interest in trying to run the crate under windows using TBS so i moved the TCTI part of this PR into #545 and added you as co-author. I hope that is ok.

When parallaxsecond#477 got merged it became possible to build using a path to the ```tpm2-tss``` installation instead of depending on ```pkg-config```. This made it possible to build under Windows. To further increase the support for the windows platform this commit moves the option for TBS TCTI that is being introduced in parallaxsecond#523 into a separate commit. This commit also updates the documentation regarding building using an installation folder. Co-author: Thomas Epperson <thomas.epperson@gmail.com> Signed-off-by: Jesper Brynolf <jesper.brynolf@gmail.com>

When parallaxsecond#477 got merged it became possible to build using a path to the ```tpm2-tss``` installation instead of depending on ```pkg-config```. This made it possible to build under Windows. To further increase the support for the windows platform this commit moves the option for TBS TCTI that is being introduced in parallaxsecond#523 into a separate commit. This commit also updates the documentation regarding building using an installation folder. Co-authored-by: Thomas Epperson <thomas.epperson@gmail.com> Signed-off-by: Jesper Brynolf <jesper.brynolf@gmail.com>

Superhepper reviewed Apr 15, 2024

View reviewed changes